Drop privileges when reading the salt file
ClosedPublic

Authored by aacid on May 16 2018, 10:56 PM.

Details

Summary

As found by Matthias Gerstner the user here controls nearly everything:

  • he controls his own password
  • he controls where the salt is read from
  • he can read the final salted hash (e.g. by calling strace() on kwalletd at the right time)

By using this fact he can do the following things:

  • test for existence of files in locations otherwise not accessible
  • exploit an information leak. 56 bytes of root owned files will be provided to him in the form of a salted hash. He won't be able to easily retrieve the original "salt" again. But if the "salt" comes from a well structured input file then the possible input combinations can suddenly be quite limited and a brute force attack can be feasible to gain knowledge of certain root-owned data.
  • the fact that the user can cause a root-owned process to read 56 bytes from an arbitrary file in the system could have other side effects depending on the situation in the system. E.g. FUSE, pseudo file systems or device files might react specially to this.

This is a very theoretical attack, but since it's reasonable easy to fix it, let's do it :)

Test Plan

kwallet-pam still works

Diff Detail

Repository
R107 KWallet PAM Integration
Branch
arcpatch-D12937
Lint
No Linters Available
Unit
No Unit Test Coverage
aacid created this revision.May 16 2018, 10:56 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 16 2018, 10:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
aacid requested review of this revision.May 16 2018, 10:56 PM
aacid added a subscriber: dakon.May 24 2018, 6:03 PM
dakon requested changes to this revision.May 25 2018, 6:03 PM
dakon added inline comments.
pam_kwallet.c
705

no init necessary, either pipe overwrites it or it returns error.

711

Use pid_t

714

leaks the pipe descriptors

720

leaks path

726

leaks pipe 1 and path

728

Just use a buffer on the stack, which can even be statically initialized (i.e. no explicit memset() needed). free(path) here.

730

Check return code. I would prefer to use open()/read()/close() directly, but that can be seen as a matter of personal preferences.

735

close pipe 1 here.

749

please use WIFEXITED and WEXITSTATUS here. The compiler is free to optimize that into a simple compare if that's what it is, but you are now relying on an implementation detail of that status code.

750

check for NULL

This revision now requires changes to proceed.May 25 2018, 6:03 PM
aacid updated this revision to Diff 34956.May 27 2018, 9:37 AM
aacid marked 7 inline comments as done.

review comments

pam_kwallet.c
730

I can check the return code but does it matter? we'll write into the pipe anyway, be it 0 or whatever we read, if it's not the correct stuff the other process will fail anyway so i don't really see the need to complicate this code more than it already is with another extra if.

750

the code i'm replacing didn't check for null, really y don't see the need, the code is half checking for null and half not, if you feel very strongly about checking for null maybe we can do it in a different patch and make sure all mallocs check for null?

aacid added inline comments.May 27 2018, 9:40 AM
pam_kwallet.c
749

we're assuming status 0 and != 0 in other places of the code, i'll suggest to also fix this separately in a new patch

dakon added inline comments.May 27 2018, 11:02 AM
pam_kwallet.c
730

Just don't write anything to the pipe, this is easily detectable at the read end. And it will not cause the reader to try to work with random data and fail in a random way, but it will just return a read error.

749

Yes please.

750

I don't see a benefit in introducing new bugs just to keep a style. And if the code does it right at some places this one should also just start right.

aacid added inline comments.May 27 2018, 3:35 PM
pam_kwallet.c
750

it's not a bug

the code works and will work 100% of times, pretending malloc fails is nice from a teoretical point of view, but it's not going to happen, if you're going to block me on this i will make you happy, but you should know you're wasting my and your time for no reason.

dakon added inline comments.May 27 2018, 7:04 PM
pam_kwallet.c
750

There can't happen anything too bad here I guess, the read() will just fail or crash.

I wonder if we should not get rid of this alltogether and just pass in a stack buffer from the caller, this would avoid any failures and leaks on it.

aacid added inline comments.May 27 2018, 7:46 PM
pam_kwallet.c
750

It's a different process, that's why we're using pipes, i don't really want to go into shared process memory for something as simple as this.

dakon added inline comments.May 27 2018, 7:58 PM
pam_kwallet.c
750

I'm talking about the salt variable, which is returned from this function. You could pass a stack buffer where this functions reads the pipe data into.

aacid updated this revision to Diff 35061.May 28 2018, 9:33 PM

make salt be on the stack
Also use better_write instead of write

dakon added a comment.May 29 2018, 4:01 PM

Nice. Now only the fread() return code needs to be handled and it is perfect.

aacid updated this revision to Diff 36575.Jun 23 2018, 5:17 PM

check fread reading the number of bytes we asked for

dakon accepted this revision.Jun 24 2018, 12:55 PM
This revision is now accepted and ready to land.Jun 24 2018, 12:55 PM
This revision was automatically updated to reflect the committed changes.