Details
- Reviewers
ManMower zmike - Commits
- rEfddcaa43c43c: OpenBSD non-PAM lokker authentication.
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.
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) | |
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...
| 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...
This is:
- a feature
- 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.