src/sulogin.c: Free previously allocated memory

[ferivoz]

The sulogin program calls pw_entry in a loop while incorrect root passwords are entered.

Free the previously allocated memory to avoid memory exhaustion.

Proof of Concept (compile with -fsanitize=address):

Run sulogin and enter wrong root passwords multiple times. Then press CTRL+D. You can see as many direct leaks as wrong passwords have been entered.

[ferivoz]

It would be nice if pw_entry could be moved into sulogin.c because no other program uses it. Also, sulogin.c could be refactored to remove its global variables.

Is there interest in this for another pull request?

[alejandro-colomar]

Reviewed-by: Alejandro Colomar <alx@kernel.org>

[hallyn]

It would be nice if pw_entry could be moved into sulogin.c because no other program uses it.

That would be good to do if we're doing this patch, because here we are depending
on the pwent being zeroed out at the start, else we'll start freeing random values.
So best not to make it seem like "just anyone" can use it.

Alternatively, we could introduce a pw_init() function which zeroes the pwent at the
top of the loop.

Also, sulogin.c could be refactored to remove its global variables.

This patch is depending on pwent being in BSS so it starts out zeroed. So don't just
move its definition into main or into the while loop without accounting for that.

Anyway, I'll note that while this 'out of memory' is not a security issue (since this
is only in sulogin, not in general library code), risking freeing uninitialized memory
is. So let's be careful with this. We've had unexpected side effects in the past from
over-zealous attempts at freeing memory in programs where it just didn't matter.

[ferivoz]

Rebased on current master. Thank you for refactoring the mentioned parts @alejandro-colomar!

[alejandro-colomar]

Thank you!

Signed-off-by: Alejandro Colomar <alx@kernel.org>

[alejandro-colomar]

Oh, you should probably keep the Co-authored-by (or Co-developed-by, as you prefer) line next to the signed-off-by. :)

https://www.kernel.org/doc/html/latest/process/submitting-patches.html?highlight=signed%20off#when-to-use-acked-by-cc-and-co-developed-by

[hallyn]

It would be nice, if anyone feels included to do more with this code, to replace void pw_entry() with int pw_entry() which returns error if we run out of memory.

Furthermore, given doc/HOWTO note on AUTOSHADOW, which is referenced in this hunk, it seems like we ought to also drop that, and just refuse to compile if AUTOSHADOW is set.

[alejandro-colomar]

It would be nice, if anyone feels included to do more with this code, to replace void pw_entry() with int pw_entry() which returns error if we run out of memory.

Yup, I had this in my plans. @hallyn

Furthermore, given doc/HOWTO note on AUTOSHADOW, which is referenced in this hunk, it seems like we ought to also drop that, and just refuse to compile if AUTOSHADOW is set.

I don't even know what AUTOSHADOW is. I've been seeing that thing and trying to ignore it. :D
I'll have a look.