Page MenuHomePhabricator

OpenBSD non-PAM lokker authentication.
ClosedPublic

Authored by netstar on Aug 1 2016, 10:18 AM.

Diff Detail

Repository
rE core/enlightenment
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
netstar updated this revision to Diff 9680.Aug 1 2016, 10:18 AM
netstar retitled this revision from to OpenBSD non-PAM lokker authentication..
netstar updated this object.
netstar edited the test plan for this revision. (Show Details)
ManMower requested changes to this revision.Aug 17 2016, 8:26 AM
ManMower added a reviewer: ManMower.
ManMower added a subscriber: ManMower.

I'm very concerned about the pipe read stuff, as I think any user (on openbsd) will be able to use enlightenment_sys -z and overflow its buffer...

src/bin/e_auth.c
176

I'd save strlen(passwd) here so you can use memset later when you're erasing it...

182

Do you really need a copy of this? if passwd needs a \n on the end can't you just ecore_exe_send() a \n after sending the password?

Otherwise, you should probably go to the trouble of erasing this string too when you're done with it?

183

Shouldn't exe be tested for validity at this point?

194

Doesn't this ensure that exe is leaked on success?

198

If you'd saved strlen(passwd) earlier you could just memset now...

src/bin/e_sys_main.c
72

A quick google search for these functions says that pw_ent->pw_passwd is a char * and crypt_checkpass() takes a const char * as a parameter...

what's with the struct passwd *pwdhash temp var?

would return crypt_checkpass(guess, pw_ent->pw_passwd); not work?

(Note: I'm not an OpenBSD developer :)

86

It seems like maybe setuid programs should make sure they're not reading an unlimited amount of data from a pipe into a 4096 byte buffer...

This is pretty poor pipe read code - it can be done directly into the buffer multiple bytes at a time without the temp char.

Also, I think the \n is probably something we shouldn't require on either end of the pipe code - the closing of the pipe should be enough. that gets us out of having to make that temp copy of the passwd with a \n on the end too?

96

why are we strdup()ing this?

100

Is this legit E/EFL coding style? (declaring a var in the middle of a block)

(but if we don't stdrup guess, I think we can avoid the temp var entirely anyway)

This revision now requires changes to proceed.Aug 17 2016, 8:26 AM
netstar updated this revision to Diff 9773.Aug 17 2016, 12:41 PM
netstar edited edge metadata.

Changes made based on recommendations.

ecore_exe_free should be handled elsewhere.

The pipe read should be okay???

Removed unnecessary uses on heap.

Added check for return on ecore_exe functions.

Regarding not terminating on EOF this was something @raster suggested to me.

So keeping the newline termination for the request to e_sys_main...

Also regarding memset that will be optimized out.

See log of commits to e_auth.c.

netstar marked 6 inline comments as done.Aug 17 2016, 12:46 PM
netstar added inline comments.
src/bin/e_auth.c
194

No

ecore_exe_free is handled in other process???

would e_util_memclear() be optimized out? I'd rather see that re-used than writing a manual loop.

You're right - the pipe read isn't dangerous, I missed the break. (I still think reading single bytes isn't the greatest use of time, but there's not really much need for optimization here I guess)

I assumed ecore_exe_free() created a local object that needed to be explicitly freed in the local process. I could be wrong. :)
Since raster's on CC now he probably knows that stuff far better than I...

netstar marked 5 inline comments as done.Aug 17 2016, 1:23 PM

thanks for your time @ManMower :)

i don't have anything more to commment on this myself :)

This revision was automatically updated to reflect the committed changes.
netstar added a subscriber: simotek.Sep 8 2016, 8:48 AM

@simotek
Hi not sure if the same applies as to efl backports. Do let me know!

Hi,

Can this possibly be backported?

zmike added a comment.Dec 1 2016, 8:02 AM

This is:

  1. a feature
  2. something which affects a suid executable

The first item would be very likely to prevent it from being backported, but the combination of the two makes it a certainty that this will not make it into an E21 release since it would then trigger additional security reviews downstream.

Thanks for the clarification there.