Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sasl: fix sqlite3 plugin value inserts #753

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sasl: fix sqlite3 plugin value inserts #753

wants to merge 1 commit into from

Conversation

SyntevoAlex
Copy link
Contributor

@SyntevoAlex SyntevoAlex commented Dec 25, 2022 •
edited
Loading

Consider the following configuration:

sqlite3 database sqlite.db:

CREATE TABLE data(username TEXT, realm TEXT, property TEXT, value TEXT)

saslpasswd.conf file

pwcheck_method: auxprop
mech_list: CRAM-MD5
auxprop_plugin: sql
sql_engine: sqlite3
sql_database: sqlite.db
sql_select: SELECT value FROM data WHERE username = '%u' AND realm = '%r' AND property = '%p'
sql_insert: INSERT INTO data (username, realm, property, value) VALUES ('%u', '%r', '%p', '%v')
sql_update: UPDATE data SET value='%v' WHERE username = '%u' AND realm = '%r' AND property = '%p'

Now, when using saslpasswd2 -u TestRealm -c TestUser it pretended to succeed, but in fact did nothing.
Worse yet, it's not just about saslpasswd2. In fact, sqlite3 auxprop couldn't insert any new data in the db.

Please refer to commit message for the explanation.

@SyntevoAlex
sasl: fix sqlite3 plugin value inserts
1f3ece4 
How it was:
* When there are no matching records, `SELECT` returned `SQLITE_OK`.
* This caused `_sqlite3_exec()` to return 0 (success).
* This caused `sql_auxprop_store()` to believe that record exists and
  `UPDATE` is needed.
* `UPDATE` didn't find a matching record to update and returned success.
* Plugin believed that it did the work, whereas it did nothing.

Now:
* `_sqlite3_exec()` returns error when `SELECT` didn't find anything.
* This is in line with other SQL implementations, as I understand them.
* Also moved string copying into callback. This fixes memory leak when
  there are multiple values, or when caller didn't give buffer for value.

Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
@SyntevoAlex
Copy link
Contributor Author

Could be related: https://lists.andrew.cmu.edu/pipermail/cyrus-sasl/2020-July/003283.html

@quanah quanah added this to the 2.2.0 milestone Jul 18, 2023
@quanah quanah requested a review from hyc July 22, 2023 22:54
hyc
hyc reviewed Aug 24, 2023
View reviewed changes
plugins/sql.c
}

/* now get the result set value and value_len */
/* we only fetch one because we don't care about the rest */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting more than one result should be a failure, certainly it is a security vulnerability to have ambiguous credentials. If an attacker can insert their own creds in front of the valid ones, etc...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hyc hyc hyc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@SyntevoAlex @hyc @quanah