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...
I'd save strlen(passwd) here so you can use memset later when you're erasing it...
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?
Shouldn't exe be tested for validity at this point?
Doesn't this ensure that exe is leaked on success?
If you'd saved strlen(passwd) earlier you could just memset now...
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 :)
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?
why are we strdup()ing this?
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.
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...
- 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.