Remove const specifiers from member variables of serializable classes, to avoid undefined behaviour

[daira]

Consider for example https://github.com/zcash/zcash/blob/zc.v0.11.2.latest/src/primitives/transaction.h#L318 :

class CTransaction
{
    //...
    const int32_t nVersion;
    //...
    template <typename Stream, typename Operation>
    inline void SerializationOp(Stream& s, Operation ser_action, int nType, int nVersion) {
        READWRITE(*const_cast<int32_t*>(&this->nVersion));
        //...
    }
}

This invokes undefined behaviour on deserialization, because a const variable is modified via a non-const access path. Edit: confirmed by http://en.cppreference.com/w/cpp/language/const_cast

We probably get away with this when compiling with gcc.

[daira]

I think we should just strip all of the const specfiers from member variables of serializable classes. They are not actually const, so the declaration is misleading in practice, as well as being formally incorrect. This will allow us to remove the *const_cast<type *>(...) hacks in SerializationOp functions.

[nathan-at-least]

Wow, gross.

Ok, so one line of thought is: if we know gcc's behavior (and we're fairly confident that won't change), and we decide to explicitly preclude other compilers which have different behavior, then I would advocate keeping the code as is, because I believe it's trying to achieve a shoddy version of rust's unsafe. To paraphrase: "these fields are immutable generally, but we make an exception for deserialization".

Removing the const keyword changes the API so that now the fields are not "generally" immutable, and downstream code may begin to mutate members.

Another line of reasoning is that C++ undefined behavior is not an acceptable platform for financial transactions. That's a pretty persuasive argument. However, I fear making this change would be messy, but moreover "go against the grain" of the codebase. Is that fear unfounded?

And finally, what is up with c++?! Why couldn't a write-through-a-const-violating-path behavior be defined to be identical to a non-const variable write? (I know there is some sausage grinding story someone while gleefully share with me…)

[nathan-at-least]

We all agree that this is a terrible state of affairs, but we may not have the bandwidth before launch to address this directly. Our next step is to see if upstream also considers this a problem and see if we can get the broader community help fix this.

[daira]

const member variables can only validly be initialized in an initialization list: http://stackoverflow.com/a/16089757/393146

[sipa]

So a bit of history behind this code:

• We needed a mechanism to cache txids, because they were being recomputed dozens of times.
• The initial solution (a cached mutable uint256 field, that was retrieved and updated from GetHash) causes many problems inside signing code which modified CTransacrion objects.
• The solution was to split into CMutableTransaction and CTransaction (immutable, and with a precomputed hash).
• To enforce that fields were not modified after the hash was assigned, the fields were all made const. Unfortunately, the deserialization code does not construct objects in place, but constructs and then updates, so the const_casts were needed.

Note that the deserialization logic does use const_casts internally already (the SerializationOp method is always non-const, and deserializing a const object works by NCONST_PTR'ing the object before calling SerializationOp).

I think this PR is safe in that it always results in the expected behavior, but it removes the safeguards against accidental mutation of CTransaction objects.

A full solution would either be making the fields of CTransaction private and using getters instead, or overhauling the serialization framework so that it can construct objects with const fields set correctly at construction time.

[zookozcash]

Thanks for your help, sipa!

[daira]

There seems to be a misconception here about how const works. It's a common misconception, arguably due to a serious design flaw in C++. The most useful thing for const to do would be simply to warn the programmer when a program is not const-correct. That is what many programmers believe it is for. The actual purpose of const intended by the specifiers of the standard, is to allow a compiler to make optimizations under the assumption that the const annotations are correct. If they're not correct, the behaviour is undefined.

Trying to use const to express "logical" immutability is a programming error, and must be corrected in any security-oriented software.

[daira]

In particular, "deserializing a const object [] by NCONST_PTR'ing the object before calling SerializationOp" is also a bug.

[sipa]

@daira I'm not disagreeing with you, just giving the history behind it and my view on how to fix it.

[daira]

The point I was trying to make was that fixing the undefined behaviour shouldn't block on a better solution such as using accessors. (I actually made a start on changing the fields to private or protected and all the clients to use accessors, and it affected so much code that I abandoned doing that.)

[daira]

I will rebase this to follow @sipa's upstream version (modulo Zcash changes) when/if that is merged.

[nathan-at-least]

Let's document in our security-warnings.md this issue exists, and users may be vulnerable when building with different versions of g++ or flags, but let's not make a change until after 1.0.

[nathan-at-least]

I'm removing this from the agenda (we talked at length last week) and adding needs prioritization so that it will be included in the security triage. I'll decide on it's triage status in the context of the full set of issues.

Note: our team disagreed about the importance of this issue. If we don't have it scheduled to be fixed by 1.0, let's gather it and other such "controversial security issues" into a list that we publicly point to as examples of security risk.

[daira]

I'll do the rebase of @sipa's fix since that will make this more likely to get through triage, and should only take me half an hour or so.

[nathan-at-least]

As you can see from milestone edits, I flip-flopped a bit here. I've moved this past 1.0.0 release because of Bitcoin's history of successful deployment.

@daira: if you find this unacceptable, would you accept a public mention in a "risk assessment" blog post of known security issues, as well as mentioning it in the security-warnings.md?

[daira]

I honestly think it is much better to just fix this, as Bitcoin is doing. (The upstream PR has three utACKs including from me, although it hasn't been merged yet.)

[sipa]

@daira See #8580 for why.

[nathan-at-least]

Removing "needs prioritization" because this is in our 1.0.1 milestone.

[daira]

I disagree with the approach in upstream's replacement patch since it does not solve the problem: https://github.com/bitcoin/bitcoin/pull/8580#issuecomment-243011281 I think we should use the approach of https://github.com/bitcoin/bitcoin/pull/8451 instead. We can always change it again if and when upstream fix the UB.

[Edit: actually they did fix that objection. I haven't yet reviewed how they fixed it.]

[daira]

https://github.com/bitcoin/bitcoin/pull/8451 is incredibly invasive; trying to cherry-pick https://github.com/bitcoin/bitcoin/pull/8580/commits/9e64b72820f719bcf0dad1ac4006fe5dbd7093cf results in a shedload of conflicts because most of that code has been touched by segwit. (The way that git does cherry-picks exacerbates the problem.)

[bitcartel]

Related https://github.com/zcash/zcash/pull/982

[daira]

I will look at the upstream patch, which might not be as invasive as I originally thought (or my threshold for invasiveness was low because I was unfamiliar with the code).