Ensure error messages don't leak private key #11523
Conversation
src/libutil/util.hh
Outdated
| std::string base64Encode(std::string_view s); | ||
| std::string base64Decode(std::string_view s); | ||
|
|
||
| /** | ||
| * Decode arbitrary bytes to Base64. | ||
| * | ||
| * @param hideValue Avoid displaying the raw Base64 in error messages, | ||
| * e.g. to avoid leaking private keys. | ||
| */ | ||
| std::string base64Decode(std::string_view s, bool hideValue = false); |
There was a problem hiding this comment.
@cole-h e.g. these type signatures (strings on both sides) are also not instructive
Mic92
added
backport 2.18-maintenance
Ericson2314
force-pushed
the
base64Decode-no-leak-private-key-on-error
branch
from
18f855c
to
d8b3626
Compare
|
Add release blocker label because this fixes a minor security regression that's not been released yet. |
|
To be honest, I feel that this is too much work to get slightly more useful error messages in some corner cases. (This makes #8766, a one-line patch, explode into 241 lines...) So I would be somewhat in favor of reverting #8766 than adding this much complexity. It might be better to improve those error messages by adding appropriate error contexts (e.g. "while parsing the extra-substituters of flake.nix: ..."). |
|
Most of the changes here already introduce error traces, so we would only need to remove the sensitive flag. |
edolstra
removed
backport 2.18-maintenance
|
@edolstra Well something along these lines would be very useful to debug a hydra failure were facing. I do agree this extra parameter is ugly, so I think I will do it all with error traces instead. |
Ericson2314
force-pushed
the
base64Decode-no-leak-private-key-on-error
branch
from
d8b3626
to
730afba
Compare
github-actions
bot
added
the
fetching
… is detected" We have a safer way of doing this. This reverts commit dc3ccf0.
Ericson2314
force-pushed
the
base64Decode-no-leak-private-key-on-error
branch
2 times, most recently
from
ecf3513
to
0cd6442
Compare
|
OK, I reverted the other PR and then added a bunch of traces where it is safe to do so. |
src/libfetchers/git-utils.cc
Outdated
| try { | ||
| keyDecoded = base64Decode(k.key); | ||
| } catch (Error & e) { | ||
| e.addTrace({}, "While decoding public key '%s' used for git signature", k.key); |
There was a problem hiding this comment.
| e.addTrace({}, "While decoding public key '%s' used for git signature", k.key); | |
| e.addTrace({}, "while decoding public key '%s' used for git signature", k.key); |
for consistency with other trace messages.
src/libutil/signature/local-keys.cc
Outdated
| try { | ||
| sig2 = base64Decode(sig); | ||
| } catch (Error & e) { | ||
| e.addTrace({}, "While decoding signature '%s'", sig); |
There was a problem hiding this comment.
| e.addTrace({}, "While decoding signature '%s'", sig); | |
| e.addTrace({}, "while decoding signature '%s'", sig); |
| if (!sensitiveValue) | ||
| extra = fmt(" with raw value '%s'", key); |
There was a problem hiding this comment.
Do we still need sensitiveValue? I think the context gives enough info.
There was a problem hiding this comment.
Eh it's pretty self-contained here like the other version (The Key constructor is now protected so it won't get used elsewhere).
I am inclined to keep it also because the thing I need to debug is a public key not private key.
Since NixOS#8766, invalid base64 is rendered in errors, but we don't actually want to show this in the case of an invalid private keys. Co-Authored-By: Eelco Dolstra <[email protected]>
Ericson2314
force-pushed
the
base64Decode-no-leak-private-key-on-error
branch
from
0cd6442
to
2b6b03d
Compare
Ericson2314
dismissed
roberth’s stale review
I hope with the new code being much more local, we don't need the test as badly. Also many things to do / me short on time.
Motivation
Since #8766, invalid base64 is rendered in errors, but we don't actually want to show this in the case of an invalid private keys.
Context
#8766
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.