agetpass(): Allocate on the stack (alloca(3)) #1191
Conversation
alejandro-colomar
force-pushed
the
agetpass
branch
4 times, most recently
from
cafa934 to
1154d32
Compare
alejandro-colomar
force-pushed
the
agetpass
branch
4 times, most recently
from
f6aeb0a to
b7c072b
Compare
alejandro-colomar
force-pushed
the
agetpass
branch
from
4545946 to
30b50b1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
alejandro-colomar
force-pushed
the
agetpass
branch
from
30b50b1 to
f024191
Compare
|
I've rewritten the patches from scratch as v4. |
alejandro-colomar
force-pushed
the
agetpass
branch
from
f024191 to
2f18365
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alejandro-colomar
force-pushed
the
agetpass
branch
2 times, most recently
from
57b8ac5 to
c7ea351
Compare
alejandro-colomar
force-pushed
the
agetpass
branch
from
c7ea351 to
0a5c77e
Compare
alejandro-colomar
force-pushed
the
agetpass
branch
2 times, most recently
from
fb69c8d to
d6ceb22
Compare
This simplifies the agetpass() call into a single line. Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar
force-pushed
the
agetpass
branch
3 times, most recently
from
1d7a2ea to
c649022
Compare
alejandro-colomar
force-pushed
the
agetpass
branch
7 times, most recently
from
2825292 to
2a5cf77
Compare
|
@jubalh Do you know why the opensuse CI is failing? It's something about the package manager, it seems. |
|
Thank you for your work on these patches. I've started reviewing them and I have a few questions to help me better understand the overall goal. I appreciate you've been working to improve the project and introduce new APIs for things like password handling. While I understand the initial need for these APIs, I'm finding the frequent modifications a bit challenging to follow. Can you provide some more context around the long-term vision for these APIs? Understanding the ultimate goal would help assess the individual changes and provide more meaningful feedback. |
The goal of the password APIs is that
Currently, we read the passwords with agetpass(), which provides the first guarantee and the third, but not the second. This has another problem: there might be truncation or buffer overflows if the buffers don't match in size. The solution to that is using lib/pass/ APIs when handling those copies too. |
|
Here's the list of APIs that I want to have: // There is also a limit in PAM (PAM_MAX_RESP_SIZE), currently set to 512.
#ifndef PASS_MAX
# define PASS_MAX (BUFSIZ - 1)
#endif
// Similar to getpass(3), but free of its problems, and get the buffer in $1.
#define getpass2(buf, prompt) readpass(buf, prompt, RPP_REQUIRE_TTY)
#define getpass2_stdin(buf) readpass(buf, NULL, RPP_STDIN)
// Similar to getpass(3), but free of its problems, and using alloca(3).
#define getpassa(prompt) getpass2(passalloca(), prompt)
#define getpassa_stdin() getpass2_stdin(passalloca())
#define passalloca() ALLOCA(1, pass_t)
#define passcalloca() passdupa(&(pass_t){""})
#define passdupa(pass) passcpy(passalloca(), pass)
typedef typeof(char [PASS_MAX + 2]) pass_t;
ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *passcpy(pass_t *restrict dst, const pass_t *restrict src);
ATTR_STRING(1)
pass_t *passzero(pass_t *pass);
ATTR_MALLOC(passzero)
ATTR_STRING(2)
pass_t *readpass(pass_t *restrict pass, const char *restrict prompt, int flags);of which, |
Hmmm, since most of them are one-liners, and only a couple of functions, maybe I can keep a single pair of .c/.h files. |
alejandro-colomar
force-pushed
the
agetpass
branch
from
2a5cf77 to
642a53f
Compare
Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar
force-pushed
the
agetpass
branch
from
642a53f to
6882d61
Compare
|
@ikerexxe , I've simplified the PR significantly, by reducing the number of files and commits (but keeping the same APIs). This should be easier to review, I think. |
alejandro-colomar
force-pushed
the
agetpass
branch
3 times, most recently
from
d15e9a3 to
81cff2a
Compare
|
The commit messages don't explain why you're adopting an While it might reduce exposure on the heap, is there any evidence this is an actual problem compared to the risks of: a) rewriting a tonne, and b) the known pitfalls of (Sorry for editing, I thought I was quick enough.) |
It is explained: Although maybe I should move that to the begining of the commit message, instead of having it in the description of a specific macro. What do you think? |
|
I would have it as top-level in the commit message, then underneath say you're describing the APIs, yeah. |
The benefits are not in this PR, but are what I have in a patch set locally (not yet in a PR): I will use these APIs more, to avoid using strtcpy() and other string APIs to handle passwords, which will itself add guarantees (via These benefits are not immediate, since we don't currently have bugs, AFAIR/AFAICS. It would be more of a guarantee that a change in the future doesn't break that. It would also make more visible all code that handles passwords, by grepping pass_t. The risks of introducing bugs in refactors are always there, on the other hand. So far, I think the benefits have been more than the problems introduced by my changes. Indeed, I had to write a summary (to apply for a grant) about the changes I had applied in the last 4 years, and the bugs fixed were a lot more than the ones I introduced (counting those that have been found, of course). The more we introduce safer APIs, the more difficult it will be to introduce bugs while refactoring, so this should be less dangerous now than some years ago. But there's always some danger. Regarding the dangers of alloca(3), it's mainly having arbitrary sizes, or alloca(3)ting in a loop. The allocation in a loop is thankfully caught by static analyzers. And we use a constant size. I don't think there are many more dangers in there. |
|
Thanks. I think I like it, but I haven't gone over all the uses yet. |
Thanks! Please let me know when you've finished reviewing. BTW, I'll be at FOSDEM. See you there? :-) |
|
I will do -- and yes! Let's make sure we find each other! :) |
These APIs will minimize the visibility of passwords, by not using the heap. The stack should have enough space for PASS_MAX+2 allocations, so this should be safe. PASS_MAX: Move definition to <pass.h> pass_t: Type to hold passwords. readpass(): readpassphrase(3) is hard to use correctly. Wrap correct usage of readpassphrase in this API. passzero(): Trivial memzero() wrapper that destroys passwords. passalloca(): This macro will allow using alloca(3) memory in these APIs. getpass2(), getpass2_stdin(): These macros are like getpass(3), but get the buffer as a parameter, avoiding the problems of getpass(3). The buffer size is fixed, and can't be overflowed. getpassa(), getpassa_stdin(): These are similar to the above, but the memory is allocated with alloca(3). Signed-off-by: Alejandro Colomar <[email protected]>
And getpassa_stdin() instead of agetpass_stdin(). Now all passwords live in the stack, and are never copied into the heap. This introduces a subtle issue: while it's fine to call malloc(3) in a loop, it is dangerous to call alloca(3) in a loop (since there's no way to free that memory). The next commit will fix that. I've addressed it in a separate commit, for readability. This alloca(3)-based API means we need to call passalloca() before a loop. Calling passalloca() (which is a wrapper around alloca(3)) in a loop is dangerous, as it can trigger a stack overflow. Instead, allocate the buffer before the loop, and run getpass2() within the loop, which will reuse the buffer. Also, to avoid deallocator mismatches, use `pass = passzero(pass)` in those cases, so that the compiler knows that 'pass' has changed, and we're not using the password after zeroing it; we're only re-using its storage, which is fine. Signed-off-by: Alejandro Colomar <[email protected]>
In the last commit, we replaced all of these calls by alloca(3)-based variants. Signed-off-by: Alejandro Colomar <[email protected]>
alejandro-colomar
force-pushed
the
agetpass
branch
from
81cff2a to
e007e8f
Compare
Hi!
This is rather sensitive, and I'd like to have as many eyes as possible look at this code.
Cc: @hallyn , @ikerexxe , @stoeckmann , @thalman , @thesamesam , @ferivoz , @jubalh
Reasons for all this change:
See Clear plaintext passwords in more error cases #1190 (comment).
Revisions:
v2
v2b
v3
v3b
v4
[[gnu::malloc()]], due to https://inbox.sourceware.org/gcc/dese7p5pdgne5gtumus6mc6ydlfcnwneeovsujpbvwqibe52ax@sl3uip7dwxg6/T/.v5
<pass/limits.h>. This breaks a circular include.v6
v6b
v6c
v7
This fixes an accidental bug I had introduced earlier. In src/sulogin.c, I was passing a NULL to passzero().
Code is also much simpler (and safer) when you can pass NULL to destructor APIs.
v7b
v7c
v8
v8b
v9
v10
v10b
restrict.v11
v12
v13
v13b
Comparison against v12, as a sanity check:
v14
lib/pass.hlib/pass.c.v15
v15b
v16
pass_t.v17
v17b