Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress false positive warning about uninitialized entropy buffers #17627

Open
wants to merge 1 commit into
base: master
from

Conversation

@Sjors
Copy link
Member

Sjors commented Nov 28, 2019

The memory sanitizer cannot see through syscall (see google/sanitizers#852). This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

Given recent problematic real issues with uninitialized variables, I think it's worth getting rid of the false positives so developers, and maybe Travis, can run the sanitizer.

==18287==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x561f1c40bfc8 in ReadBE64(unsigned char const*) /home/dev/bitcoin/src/./crypto/common.h:67:12
    #1 0x561f1c3fe1d3 in (anonymous namespace)::sha512::Transform(unsigned long*, unsigned char const*) /home/dev/bitcoin/src/crypto/sha512.cpp:59:63
    #2 0x561f1c3fcb77 in CSHA512::Write(unsigned char const*, unsigned long) /home/dev/bitcoin/src/crypto/sha512.cpp:168:9
    #3 0x561f1c2d102d in CSHA512& (anonymous namespace)::operator<<<timespec>(CSHA512&, timespec const&) /home/dev/bitcoin/src/randomenv.cpp:119:12
    #4 0x561f1c2d07e8 in RandAddDynamicEnv(CSHA512&) /home/dev/bitcoin/src/randomenv.cpp:249:12
    #5 0x561f1c2cbfb9 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) /home/dev/bitcoin/src/random.cpp:513:5
    #6 0x561f1c2c933e in ProcRand(unsigned char*, int, RNGLevel) /home/dev/bitcoin/src/random.cpp:553:9
    #7 0x561f1c2c8f98 in GetRandBytes(unsigned char*, int) /home/dev/bitcoin/src/random.cpp:558:59
    #8 0x561f1baa521e in (anonymous namespace)::CSignatureCache::CSignatureCache() /home/dev/bitcoin/src/script/sigcache.cpp:34:9
    #9 0x561f1b3bcdf9 in __cxx_global_var_init.3 /home/dev/bitcoin/src/script/sigcache.cpp:67:24
    #10 0x561f1b3bce88 in _GLOBAL__sub_I_sigcache.cpp /home/dev/bitcoin/src/script/sigcache.cpp
    #11 0x561f1c60107c in __libc_csu_init (/home/dev/bitcoin/src/bitcoind+0x129507c)
    #12 0x7fc924a8fb27 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:266
    #13 0x561f1b3c3659 in _start (/home/dev/bitcoin/src/bitcoind+0x57659)

  Uninitialized value was stored to memory at
    #0 0x561f1b3c9316 in __msan_memcpy (/home/dev/bitcoin/src/bitcoind+0x5d316)
    #1 0x561f1c3fcc8c in CSHA512::Write(unsigned char const*, unsigned long) /home/dev/bitcoin/src/crypto/sha512.cpp:179:9
    #2 0x561f1c2cbc50 in SeedSlow(CSHA512&) /home/dev/bitcoin/src/random.cpp:467:12
    #3 0x561f1c2cbf65 in SeedStartup(CSHA512&, (anonymous namespace)::RNGState&) /home/dev/bitcoin/src/random.cpp:509:5
    #4 0x561f1c2c933e in ProcRand(unsigned char*, int, RNGLevel) /home/dev/bitcoin/src/random.cpp:553:9
    #5 0x561f1c2c8f98 in GetRandBytes(unsigned char*, int) /home/dev/bitcoin/src/random.cpp:558:59
    #6 0x561f1baa521e in (anonymous namespace)::CSignatureCache::CSignatureCache() /home/dev/bitcoin/src/script/sigcache.cpp:34:9
    #7 0x561f1b3bcdf9 in __cxx_global_var_init.3 /home/dev/bitcoin/src/script/sigcache.cpp:67:24
    #8 0x561f1b3bce88 in _GLOBAL__sub_I_sigcache.cpp /home/dev/bitcoin/src/script/sigcache.cpp
    #9 0x561f1c60107c in __libc_csu_init (/home/dev/bitcoin/src/bitcoind+0x129507c)

  Uninitialized value was created by an allocation of 'buffer' in the stack frame of function '_ZL8SeedSlowR7CSHA512'
    #0 0x561f1c2cbb80 in SeedSlow(CSHA512&) /home/dev/bitcoin/src/random.cpp:459

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/dev/bitcoin/src/./crypto/common.h:67:12 in ReadBE64(unsigned char const*)
Exiting

See #17620 for how to use the memory sanitizer.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 28, 2019

I think in general, initializing everything unless it's deliberately not done for performance reasons, is a good thing. One company I've worked for even had a strict "initialize everything" policy, no exceptions.

Fixing random buffers because of valgrind false positives has some very bad precedent though, please review carefully.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2019

@laanwj To be fair this is not a Valgrind false positive? :) Valgrind false positives are very rare in my experience.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 28, 2019

To be fair this is not a Valgrind false positive? :) Valgrind false positives are very rare in my experience.

It's a false positive because the buffer is actually filled in by the syscall but valgrind msan doesn't notice.

The memory sanitizer cannot see through syscall (see google/sanitizers#852). This caused a false positive use-of-uninitialized-value error for SYS_getrandom.

Oh you're right, wrong tool.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2019

@laanwj Yes, but the false positive is in MemorySanitizer -- not Valgrind :)

@elichai

This comment has been minimized.

Copy link
Contributor

elichai commented Nov 28, 2019

Concept ACK.
From a quick glance it seems like the only change is initializing the value, and because unlike the openssl/debian fiasco, we don't use unintialized memory for generating randomness then there's no logical reason why this can make anything that currently works break (unless it was already broken).

Still requires close up review but I think it's good to keep that in mind

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 28, 2019

Agree that it needs careful review. I can replace the {0} with "// intentionally not initialized" where needed.

Also note that the code this touches is relatively new (added by @sipa in #15250 and #17270), and deserves extra scrutiny anyway now that we've dropped the OpenSSL training wheels.

The actual initialization before this commit took place in GetOSRand, which is different depending on the operating system details. That's probably another good reason to make sure stuff is initialized before it goes in there.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2019

Concept ACK

I think in general, initializing everything unless it's deliberately not done for performance reasons, is a good thing. One company I've worked for even had a strict "initialize everything" policy, no exceptions.

Agree. I think it is worth doing.

Some background reading that might be of interest regarding the pros/cons of default initialization in general:

Some quotes:

  • Linus Torvalds: "So I'd like the zeroing of local variables to be a native compiler option, so that we can (eventually - these things take a long time) just start saying "ok, we simply consider stack variables to be always initialized".
  • Linus Torvalds: "Again - I don't think we want a world where everything is force-initialized. There are going to be situations where that just hurts too much. But if we get to a place where we are zero-initialized by default, and have to explicitly mark the unsafe things (and we'll have comments not just about how they get initialized, but also about why that particular thing is so performance-critical), that would be a good place to be."

Note that there is a clang-tidy fixup that can rewrite this for us if we want (applies only to integers, booleans, floats, doubles and pointers unfortunately): https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-init-variables.html

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2019

@Sjors Which subset of these changes are required to make MemorySanitizer happy? Not all these are technically needed, right? (I'm not opposing fixing it generally, but asking to get the full picture :))

@@ -451,7 +451,7 @@ static void SeedFast(CSHA512& hasher) noexcept

static void SeedSlow(CSHA512& hasher) noexcept
{
unsigned char buffer[32];
unsigned char buffer[32] = {0};

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 28, 2019

Author Member

@practicalswift once I fixed this one, it ran into unrelated use-of-uninitialized-value problems. That could mean this is the only problem, or the other ones just didn't get a chance to get called before the sanitizer bailed out.

Copy link
Member

jonatack left a comment

Concept ACK. I feel unqualified to ACK here, but I read the changes and the first 2 links provided by @practicalswift, built ccfed8a without sanitizer (--enable-debug only) with gcc version 8.3.0 (Debian 8.3.0-6) on Debian 4.19.37-5+deb10u2 (2019-08-08), ran tests, and am running bitcoind without issues and seeing the Feeding n bytes of dynamic environment data into RNG logs with typical values for n (~16700) on this installation.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Nov 28, 2019

Initializing this would conceal from valgrind if changes in the code cased the random variable to not get filled which would be a moderately serious problem: Zero is not an acceptable random value. :)

Since msan is actually incorrect due to a known and documented limitation in msan, might it not be better to use a suppression? I'm not a fan of suppressing actual errors-- but this isn't an error, it's a tool limit, and suppressing-by-initializing will reduce the effectiveness other tools without the same limit.

@elichai

This comment has been minimized.

Copy link
Contributor

elichai commented Nov 28, 2019

Initializing this would conceal from valgrind if changes in the code cased the random variable to not get filled which would be a moderately serious problem: Zero is not an acceptable random value. :)

Since msan is actually incorrect due to a known and documented limitation in msan, might it not be better to use a suppression? I'm not a fan of suppressing actual errors-- but this isn't an error, it's a tool limit, and suppressing-by-initializing will reduce the effectiveness other tools without the same limit.

That's a good point. does valgrind support syscalls?

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 28, 2019

Maybe I can add assert statements directly after any buffers are filled to check that they're not still 0? (though I suppose that could be forgotten in new places)

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 28, 2019

Clarification regarding my Concept ACK: a suppression would be totally fine too.

As long as we make it easier for everyone to use MemorySanitizer I'm happy: we need more MSAN testing :)

@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch 2 times, most recently from 605991f to a69bb00 Nov 29, 2019
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 29, 2019

I added asserts to ensure these buffers are actually populated on first use. I also added @gmaxwell's comment about valgrind. I'm not very familiar with how valgrind works. If it substitutes calls to hasher.Write(), hasher.Finalize(), GetOSRand(), etc with no-op, then this won't work.

I could pick a magic value instead, initialize with that and use == in the assert.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 29, 2019

That's a good point. does valgrind support syscalls?

Yes, valgrind doesn't have this issue.

Maybe I can add assert statements directly after any buffers are filled to check that they're not still 0? (though I suppose that could be forgotten in new places)

But what if the input is legitly 0? These are not random values being fed in, but just some OS data, which may or may not consist of zeroes.

So NACK on the asserts.

@@ -389,6 +391,8 @@ class RNGState {
++m_counter;
// Finalize the hasher
hasher.Finalize(buf);
// Ensure Finalize() populated buf
assert(buf);

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 29, 2019

Member

FWIW: asserting the address of the buffer, on the stack, like you do here does nothing.

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 29, 2019

Author Member

For future reference, something like this works (?), though it's hideous, probably needs a macro, which adds even more complexity:

unsigned char zeros[sizeof(buf)] = {0};
assert(!memcmp(fbuf, zeros, sizeof(buf)));

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 29, 2019

Author Member

Funny who you run into while googling that: https://rusty.ozlabs.org/?p=560 :-)

@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch from a69bb00 to 4cab23d Nov 29, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 29, 2019

I've thought about it a bit and I'm principally opposed to adding asserts here. Now you're changing run-time behavior of the application to work around a problem in an analysis tool.

I'm sorry to say this but : please fix the tool instead

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 29, 2019

Echoing the NACK on the asserts :)

@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch from 4cab23d to 605991f Nov 29, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 29, 2019

@Sjors Have you considered taking the suppressions route instead? Perhaps that is less intrusive.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 29, 2019

Reverted to the original approach, which just initializes to zero. I'm not sure if intentially leaving them uninitialized and having the warnings suppressed if the right approach either.

@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch from 605991f to 1143353 Nov 29, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 29, 2019

I'm not sure if intentially leaving them uninitialized and having the warnings suppressed if the right approach either.

Why? :)

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 29, 2019

It seems like we're choosing between two tools here:

  1. memory sanity check: = {0}
  2. valgrind: // intentionally undefined to allow valgrind to catch issues

There's gotta be a solution that involves defined behavior. That would make the code easier to reason about, and prevents folks on different operating systems and compiler optimization flags not noticing bugs (as happened with #17568 and #17449).

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 29, 2019

In that case, let's at least not penalize the tool that actually gets things right (valgrind).

So I'd vote for adding a MSAN surpression and just be done with it. It can be removed when the tool is fixed.

@jnewbery jnewbery mentioned this pull request Dec 2, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 3, 2019

NACK. We don't generally change code to please buggy tool or compiler warnings, especially if the "fix" hides actual bugs in our code base.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Dec 4, 2019

@MarcoFalke it's about more than making tools happy. The tool pointed us to these uninitialized variables, which have caused pretty serious bugs in the recent past and are bad practice. I don't think we should just ignore this.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 4, 2019

@Sjors You haven't addressed the question about suppressions. Why not suppress this case? :)

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2019

I agree with @MarcoFalke and @practicalswift here.

As I said before, if we do anything here it should be a tool-specific suppression, until the tool is fixed.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Dec 4, 2019

I'm not against suppressing the warning, but I don't think that's enough by itself. It doesn't make the undefined behavior go away. Perhaps one day the address sanitizer can reason about undefined behavior even through syscall. That's great, but still not a good reason to keep the undefined behavior.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 4, 2019

But it is not undefined behavior, this specific tool is unable to reason through this. If it was actual undefined behavior, we'd respond very differently.

Sjors added a commit to Sjors/bitcoin that referenced this pull request Dec 4, 2019
Memory sanitizer cannot see through syscall. This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

After discussion in bitcoin#17627 it was decided not to initialize
these variables to 0, but instead to suppress the warning.
@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch 2 times, most recently from 4777647 to 53460b4 Dec 4, 2019
Sjors added a commit to Sjors/bitcoin that referenced this pull request Dec 4, 2019
Memory sanitizer cannot see through syscall. This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

After discussion in bitcoin#17627 it was decided not to initialize
these variables to 0, but instead to suppress the warning.
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Dec 4, 2019

I added the suppression, as narrowly as possible. I also added comments to the uninitialized variables, pointing back to this discussion so it doesn't get repeated.

Sjors added a commit to Sjors/bitcoin that referenced this pull request Dec 4, 2019
Memory sanitizer cannot see through syscall. This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

After discussion in bitcoin#17627 it was decided not to initialize
these variables to 0, but instead to suppress the warning.
@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch from 53460b4 to 527f3ec Dec 4, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 4, 2019

@MarcoFalke it's about more than making tools happy. The tool pointed us to these uninitialized variables, which have caused pretty serious bugs in the recent past and are bad practice. I don't think we should just ignore this.

Again, this is a bug in the tool. These variables are not uninitialized when read. Initializing them with the wrong value will cause bugs in the future because tools that properly detect uninitialized reads will no longer yell at you.

This point is repeated every couple of months. Might be a good candidate to add to the dev notes.

@Sjors Sjors changed the title Initialize entropy buffers to suppress false positive warning Suppress false positive warning about uninitialized entropy buffers Dec 4, 2019
# if __has_feature(memory_sanitizer)
__attribute__((no_sanitize("memory")))
# endif
#endif

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 4, 2019

Member

This is a clang bug, right? Should link to the commit that fixed it. And since you are compiling clang from source you might as well use that commit and then remove this code blob here.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 4, 2019

Author Member

The memory sanitizer appears to be a Google project, and they don't expect syscall support to be fixed anytime soon. Only a more narrow case of getrandom was fixed, which is of no use to us: google/sanitizers#852 (comment)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 5, 2019

Member

Ok, fine. I'd still prefer a suppression in a suppressions file if possible. Modifying the code like this makes it harder to read and potentially impossible to compile on some environments.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2019

Author Member

@practicalswift any idea how to achieve such a suppressions file? I could also put it in a macro, or we can do that if we ever need it again.
The outer #if defined(__has_feature) is there to make sure other environments don't trip over it.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 6, 2019

Member

@Sjors Sure! :) You need to use the sanitizer special case list functionality by passing -fsanitize-blacklist=test/sanitizer_suppressions/msan as part of CFLAGS/CXXFLAGS. You need to create the file test/sanitizer_suppressions/msan following the file format specified here: https://clang.llvm.org/docs/SanitizerSpecialCaseList.html#format

Unfortunately MemorySanitizer only supports suppression lists supplied at compile-time. Some of the other sanitizers allow suppression lists supplied at run-time.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 6, 2019

Author Member

I prefer to punt on this until we need it in more places...

This comment has been minimized.

Copy link
@laanwj

laanwj Dec 9, 2019

Member

I prefer to punt on this until we need it in more places...

So this is the only place where MSAN special treatment is needed? That's pretty good, I had assumed it would be the first of many.

This comment has been minimized.

Copy link
@Sjors

Sjors Dec 9, 2019

Author Member

I don't know yet. The binary exits once it finds a single problem. The next problem in line is in a different file, but who know what happens after that.

There's only one place in our codebase where we use syscall, so that's a reason for optimism.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 5, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 5, 2019

@Sjors Thanks for using a suppression instead. Is this the only suppression needed to make MSAN happy in your setup?

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Dec 5, 2019

@practicalswift no, it fails on other things, but those appear unrelated. I'll update #17620 later.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 6, 2019

It's not clear to me that everyone understands what's going on here. And maybe I caused some confusion by say that zero is not an acceptable random value. No constant is an acceptable random value.

The variable is not undefined. It gets defined by the SYS_getrandom. Memcheck has a flaw/shortcoming where it doesn't know or assume that sys_getrandom initializes it so it falsely reports it as uninitialized.

If it were actually undefined then that would mean that the getrandom call wasn't doing anything which would be a really serious problem because it would mean that the software getting random numbers it was expecting. Ignoring the other belt-and-suspenders randomness inputs, a failure to read random data into that variable would be a total break resulting in predictable keys.

With the code as it currently is, should some bug be added so that the variable is not initialized at least valgrind will start yelling. If the code is changed to initialize the value with anything except a truly random value (such a zero or some random hash) then if a flaw that skips the randomness call is introduced the software will be just as vulnerable but in that case the flaw will be undetectable by valgrind. Similarly, compilers can often warn when they're certain a value will be undefined, and pre-emptive dummy initialization will break those warnings.

An assertion would not be appropriate because any value is acceptable random value, so long as it's actually random and not some predictable constant.

Another potential tool in the debate between "pre-initilize variables and avoid the risk of undefined values" and "don't pre-init and preserve valgrind/memcheck sensitivity to coding errors that cause the required "real initialization" is that valgrind.h has special macros that can be used to mark memory as undefined in valgrind's tracking although they're defined for language purposes. (VALGRIND_MAKE_MEM_UNDEFINED(x,y), see secp256k1/tests.c). I'm not sure if anything similar exists for memcheck.

In some rare cases pre-initilization is a performance concern. But in most-- where the variable isn't in some inner loop and doesn't involve a bunch of malloc-- it's not. It is, however, very often a testing sensitivity concern. Of course, in some cases the code can be re-factored to do the proper 'final' initialization at declaration time then that's best.

If there were a big swing in the project towards performing dummy initialization I think it would be really useful if it were always done via a macro that would allow disabling it for testing (or valgrind annotating it).

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 6, 2019

Memcheck has a flaw/shortcoming where it doesn't know or assume that sys_getrandom initializes it so it falsely reports it as uninitialized.

You mean MemorySanitizer (MSAN) and not Memcheck (Valgrind), right? :)

In some rare cases pre-initilization is a performance concern. But in most-- where the variable isn't in some inner loop and doesn't involve a bunch of malloc-- it's not. It is, however, very often a testing sensitivity concern.

From a security perspective doesn't it boil down to a trade-off between maximising testing sensitivity (achieved by not pre-initializing) vs minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)?

Is that a fair summary? :)


Background context for others:

Depending on code generation, a programmer who runs into undefined behavior by using an uninialized automatic variable may observe any previous value (including program secrets), or any value which the compiler saw fit to materialize on the stack or in a register (this could be to synthesize an immediate, to refer to code or data locations, to generate cookies, etc).

Memory sanitizer cannot see through syscall. This caused a false
positive use-of-uninitialized-value error for SYS_getrandom.

After discussion in #17627 it was decided not to initialize
these variables to 0, but instead to suppress the warning.
@Sjors Sjors force-pushed the Sjors:2019/11/buffers branch from 527f3ec to 31408ed Dec 6, 2019
@DrahtBot DrahtBot removed the Needs rebase label Dec 6, 2019
@laanwj laanwj added the Tests label Dec 10, 2019
@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Dec 21, 2019

minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)

There is no risk of leaking anything here, and of the randomness is hashed before being used.

The difference between initializing and not is that if a bug is introduced: With initializing a bug that hobbles the random input will not be detectable (except via review), without initializing the bug would be detectable by memcheck but there would also be undefined behaviour (however, on currently supported compilers/systems the undefined behaviour would be harmless esp compared to the no-randomness bug) until someone noticed the error report in memcheck and fixed it.

@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Dec 21, 2019

FWIW I attempted to summarise this discussion in the review club notes as a resource and will probably update it with the points made here. Don't hesitate to suggest corrections or improvements in the repository or to me via IRC.

https://bitcoincore.reviews/17639.html#what-can-we-do-to-mitigate-uninitialized-variables

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 21, 2019

From a security perspective doesn't it boil down to a trade-off between maximising testing sensitivity (achieved by not pre-initializing) vs minimising the risk of leaking non-dummy memory content (achieved by pre-initializing)?

There is no risk of leaking anything here, and of the randomness is hashed before being used.

I was talking about the pros and cons of pre-initializing generally: in this specific case there is obviously no risk of leaking sensitive data :)


Recent discussion on Twitter about pre-initialization in general with some people from Apple working on iPhone security and LLVM respectively that might be of interest for readers of this thread: "If you’ve got a security-sensitive codebase, you should be using -ftrivial-auto-init=pattern in Clang. In 2020, there’s no good reason for uninitialized variables to be exploitable."

I would assume that they do developer builds without -ftrivial-auto-init=pattern (no protection against exploitability but with unchanged testing sensitivity) and release builds with -ftrivial-auto-init=pattern (guards against exploitability but loses testing sensitivity).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.