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.

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

Reset sqlite log handler when db is closed #2553

Closed

Conversation

jpalus
Copy link
Contributor

@jpalus jpalus commented Jun 26, 2023

When db is opened rpm registers rpmlog() based callback within sqlite and passes rpmdb as user data. When db is closed and rpmdb gets eventually freed it might lead to UAF if another sqlite3 db is opened different than rpm db.

It happens ie if normal user invokes dnf download. First rpm database is opened and log handler is left after closing. Than generate_completion_cache plugin tries to open /var/cache/dnf/packages.db but failes due to permission issues leading to log message attempt triggering UAF and oftentimes segfault.

When db is opened rpm registers `rpmlog()` based callback within sqlite
and passes `rpmdb` as user data. When db is closed and `rpmdb` gets
eventually freed it might lead to UAF if another sqlite3 db is opened
different than rpm db.

It happens ie if normal user invokes `dnf download`. First rpm database
is opened and log handler is left after closing. Than
`generate_completion_cache` plugin tries to open
`/var/cache/dnf/packages.db` but failes due to permission issues leading
to log message attempt triggering UAF and oftentimes segfault.
@pmatilai
Copy link
Member

Ohh, a good catch, thanks!

This isn't the right fix though, as the API allows you to have multiple databases (at different paths) open at once. The gotcha here is that unlike all/most of the surrounding code sqlite3*() calls, sqlite3_config() is global, and it's indeed plain wrong to pass the rpmdb handle there. I need to see whether there's something else in the sqlite API that would suit our purposes better, otherwise we'll just have to stop passing rpmdb to the callback.

@jpalus
Copy link
Contributor Author

jpalus commented Jun 26, 2023

Completely agree with everything you've said. I've actually had a look at sqlite API myself before creating PR and couldn't find anything that would work per database. I didn't feel qualified to make decision on stopping/altering logging within rpm either. Hence this fix.

I'd say it's not only about passing rpmdb, but logging in general. While somewhat related I don't see a reason why generate_completion_cache database errors should be logged with rpmlog. It wouldn't be logged if they were opened in different order. So it's either logging callback reset or no logging at all. Unless of course you can figure how logging can be done per database.

@pmatilai
Copy link
Member

Oh right, it's not even about one vs multiple rpmdbs, it's ANY sqlite uses in the same process. Ugh. Makes you wonder how we haven't ran into this sooner.

@pmatilai
Copy link
Member

Fixed by removing the offending sqlite3_config() call: ea3187c
There's simply no other alternative in the current sqlite API.

Thanks again for spotting and reporting!! This needs backporting to existing releases too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants