On Thu, Apr 30, 2020 at 3:34 PM Richard Smith <richardsmith_at_[hidden]> wrote:
>
> On Thu, Apr 30, 2020 at 11:57 AM Aaron Ballman <aaron_at_[hidden]> wrote:
>>
>> On Thu, Apr 30, 2020 at 2:26 PM Richard Smith <richardsmith_at_[hidden]> wrote:
>> >
>> > On Thu, Apr 30, 2020 at 11:02 AM Aaron Ballman <aaron_at_[hidden]> wrote:
>> >>
>> >> On Thu, Apr 30, 2020 at 1:52 PM Richard Smith via SG12
>> >> <sg12_at_[hidden]> wrote:
>> >> >
>> >> > On Thu, 30 Apr 2020, 07:39 Arthur O'Dwyer via SG12, <sg12_at_[hidden]> wrote:
>> >> >>
>> >> >> On Thu, Apr 30, 2020 at 2:19 AM Jens Maurer <Jens.Maurer_at_[hidden]> wrote:
>> >> >>>
>> >> >>> On 30/04/2020 02.10, Arthur O'Dwyer via SG12 wrote:
>> >> >>> >
>> >> >>> > It's easy to ensure that an operation */is/* performed; all you have to do is insert an optimization barrier like Google Benchmark's DoNotOptimize, which is basically an `asm volatile("")` statement.
>> >> >>>
>> >> >>> Yes.
>> >> >>> Paul's P1382 is much easier in this regard, and very close to traditional volatile semantics.
>> >> >>
>> >> >>
>> >> >> I wouldn't call P1382 "easier" on the human programmer; it's a lot more fiddly than a simple "escape" or "DoNotOptimize" function that can fence any side-effect at all. But it's certainly close to traditional `volatile`.
>> >> >>
>> >> >> The equivalent of `memset(passwordbuffer, 0x00, sizeof passwordbuffer)` under P1382 would be
>> >> >> char passwordbuffer[100];
>> >> >> ...
>> >> >> for (auto& ch : passwordbuffer) {
>> >> >> volatile_store(&ch, '\0'); // note that volatile_store(&ch, 0) would not compile, and I think that's too bad
>> >> >> }
>> >> >> although I think P1382 needs to explicitly explain whether
>> >> >> char sequence_of_zeros[100] = {};
>> >> >> std::volatile_store<char[100]>(&passwordbuffer, sequence_of_zeros);
>> >> >> is intended to be supported. (I predict it's not supported, because SD-8, but I'd like to see it in writing in the paper.)
>> >> >>
>> >> >>
>> >> >> Tony Van Eerd suggests:
>> >> >> > a function: volatile_clear(void *ptr, int size), that does volatile writes of 0 into the memory.
>> >> >> > Is this a worthwhile step?
>> >> >>
>> >> >> IMHO that would be a useful addition to P1382 (given the fiddliness of using `volatile_store` as shown above).
>> >> >> I would even call it `volatile_memset` with the appropriate signature, since we have prior art for memset.
>> >> >>
>> >> >> Niall Douglas suggests:
>> >> >> > I'd gladly take into the standard an:
>> >> >> > assume_escaped(const byte *, size_t);
>> >> >> > I can build stuff on top of that.
>> >> >>
>> >> >> That signature should be simply `void assume_escaped(const void*)`. The `size_t` parameter is pointless, because the entire point of this function (from the compiler's point of view) is that everything reachable from that pointer must be assumed to have escaped.
>> >> >> int a[2];
>> >> >> int *p = &a[0];
>> >> >> int *q = &a[1];
>> >> >> assume_escaped(p);
>> >> >> assume_escaped(q);
>> >> >> Those two lines should have the same effect from the compiler's point of view, since the memory at `*p` is also reachable via the expression `*(q-1)`. If `q` escapes into a-function-that-the-compiler-doesn't-know-what-it-does, then the compiler must assume that `*(q-1)` might be read by that function.
>> >> >> Here is Google's `DoNotOptimize(const T&)`.
>> >> >> If we had a simple DoNotOptimize-style library function, we wouldn't need `volatile_store` for Miguel's purpose. But I believe Paul wants `volatile_store` for more "lock-free atomic access patterns" kinds of things; `volatile_store` has other benefits for his domain, such as non-tearing.
>> >> >>
>> >> >>
>> >> >>> > memset(passwordbuffer, 0x00, sizeof passwordbuffer);
>> >> >>> > DoNotOptimize(&passwordbuffer); // escape this variable to force the memset to be respected
>> >> >>> >
>> >> >>> > What secure_clear is trying to do is to ensure that a "sufficiently dumb compiler" does not compile the above code into the equivalent of
>> >> >>> >
>> >> >>> > char *secondcopy = strdup(passwordbuffer); free(secondcopy); // oops, leaked a copy of the data onto the heap
>> >> >>> > static char secondcopy[100]; strcpy(secondcopy, passwordbuffer); // oops, leaked a copy of the data into the data section
>> >> >>> > memset(passwordbuffer, 0x00, sizeof passwordbuffer);
>> >> >>> > DoNotOptimize(&passwordbuffer); // the memset is respected, but useless
>> >> >>>
>> >> >>> And then, the compiler does
>> >> >>> strcpy(passwordbuffer, secondcopy);
>> >> >>> rigbt before "passwordbuffer" ends its lifetime. Also undesirable and
>> >> >>> totally defeating the purpose, but fine under the status quo as-if rule.
>> >> >>
>> >> >>
>> >> >> Sure; but my point was that the purpose of "not leaking the secret" has already been defeated, even if the compiler doesn't do what you said. As soon as the very first copy is made, the security has gone out the window. And those copies can be made under the as-if rule.
>> >> >
>> >> >
>> >> > Right. Rather than pursuing an option that we don't know of any way to extend to being an actually secure clear mechanism, we should find some way to express the actually desired semantics of "don't leak the secret stuff".
>> >> >
>> >> > For example, suppose we had a function attribute:
>> >> >
>> >> > [[secure_clear]]
>> >> >
>> >> > ... with no normative semantics (because it has no effect on the abstract machine), but with recommended behaviour that it ensures to the extent possible that data used in the function is not accessible after it terminates, by any means -- in particular, that any stack space or registers used by the function are cleared when the function returns or propagates an exception. That seems implementable, and seems like it would actually give the desired depth of protection.
>> >>
>> >> I'm not certain I agree, because implementations are free to
>> >> effectively ignore the attribute because a correct program would
>> >> remain correct in the absence of the attribute. Maybe we would want to
>> >> nam it 'secure_clear' as a keyword attribute, similar to what we did
>> >> for alignas (and for the same reasons).
>> >
>> >
>> > Given the way we describe the abstract machine, I think this is simply fundamental. We cannot give a guarantee here because the behavior of the physical machine is outside our scope and the behavior of the abstract machine is unaffected by the attribute. It seems entirely appropriate to me for this to be in the domain of quality-of-implementation -- whether and to what extent an implementation can bound the behavior of a program whose behavior is undefined isn't something we can reasonably regulate.
>> >
>> > (Sure, a conforming implementation can ignore the attribute. A constant expression evaluator certainly would. And that seems fine, because that implementation is emulating the abstract machine and tightly bounding the possible effects in the presence of undefined behavior, so (assuming the implementation itself doesn't have bugs) it therefore presents no risk of leaking secrets to later parts of the program execution.)
>>
>> Sorry, I wasn't sufficiently clear with explaining why I think a
>> keyword attribute is a better approach. With an attribute, an
>> implementation can ignore the attribute *entirely* (with no
>> diagnostics to the user) and still conform, and that seems like a very
>> bad property for an attribute attempting to give some best-effort
>> guarantees around security. This is a bit different than other
>> attributes we currently support, where ignoring the attribute doesn't
>> have as dangerous of an effect on correct programs. With a keyword
>> attribute, the implementation can still give the best-effort
>> guarantees through a Recommended Practice (so we still can be
>> hand-wavy like we need to be, and leave a lot up to QoI), but we can
>> still require that a conforming implementation at least parse and
>> diagnose use of the keyword attribute if their best-effort is to do
>> nothing at all. This way, users can at least expect a diagnostic if
>> they're using an implementation that elects to functionally ignore the
>> attribute.
>
>
> Two observations:
>
> 1) I think what you're suggesting is less different from adding a standard attribute than you're suggesting. Implementations cannot entirely ignore standard attributes; they are still required to implement the checks that the attributes are applied to the right kind of entity and reject if not, and perform whatever other checks are described in the specification for the attribute. An implementation that completely ignores a standard attribute (skips it without even parsing and validating it) does not conform to the standard containing that attribute, at least for all of the existing attributes.

I don't think that's quite right. I believe it is conforming for an
implementation to parse [[ where an attribute-specifier-seq can
appear, eat all balanced tokens until it reaches the terminating ]],
and issue one diagnostic saying "attributes ignored" to cover our
conformance requirements. But, I suppose for such an implementation,
the user gets the information I care about them getting for this
specific attribute. Other that that, I agree with what you're saying.

> The situation with your keyword would be largely the same; the only difference is what happens in the case where an (old or non-conforming) implementation that has not implemented the proposal in question sees the attribute: does it reject due to hitting a keyword it's never heard of, or does it (probably silently) ignore it? Maybe that's an argument in favor of a non-attribute, but you can get the same result by checking for __has_cpp_attribute and #erroring if it's unavailable, which I'd expect libraries to do anyway, so I think it's a weak argument, and maybe too weak to justify adding a keyword.

Agreed.

> 2) I think we don't need to worry too much about intentionally low-quality or non-conforming implementations. Whichever option we take, such low-quality implementations can still exist if the implementers decide that they want to go that way, and there's nothing we can do about that. All we can do is trust that the implementers will do whatever they think is in their users' best interests. That said, it might be reasonable to make the extent to which the attribute / keyword is enforced be implementation-defined, to impose at least a documentation requirement on conforming implementations so that their users can make an informed decision between implementations.

I was keeping WG14 in mind because they would likely also be
interested in such an attribute, and they support a *lot* more
implementations than WG21 does. But I think you captured the important
thing I was after: ensuring that users have *some* way to know whether
their implementation is making an effort to support the semantics of
the attribute at all.

> In any case, I don't want to get deep into the details of a design that has never been proposed or fully explored :) My point wasn't "do secure clearing in this other way", my point was "do secure clearing in some way that can actually provide the guarantees that we want; do not standardize something that we don't think can work" (with an example to demonstrate that solutions are within our grasp).

+1, thank you for raising the idea.

~Aaron

>
>>
>> ~Aaron
>>
>> >
>> >>
>> >> ~Aaron
>> >>
>> >> >
>> >> > We'd still need some way to securely clear heap allocations. Maybe the proposed std::secure_clear function would be useful for that, but there might be an argument that what we actually want is a secure allocator. For example, maybe you don't want your secrets written to a page file -- we could take care of that for the stack automatically via the function attribute, but probably not for the heap if the regular allocator is used. And you probably don't want a read-after-free elsewhere in your program to be able to read your secure secrets, which would be the case if you used the regular allocator.
>> >> >
>> >> >>> > The above is permitted today under the as-if rule; but secure_clear is asking for some way to /*ensure, formally,*/ that it /*never */happens.
>> >> >>> >
>> >> >>> > Ensuring that something /doesn't /happen is much much harder than ensuring that it /does /happen.
>> >> >>>
>> >> >>> Agreed.
>> >> >>
>> >> >>
>> >> >> _______________________________________________
>> >> >> SG12 mailing list
>> >> >> SG12_at_[hidden]
>> >> >> Subscription: https://lists.isocpp.org/mailman/listinfo.cgi/sg12
>> >> >> Link to this post: http://lists.isocpp.org/sg12/2020/04/0886.php
>> >> >
>> >> > _______________________________________________
>> >> > SG12 mailing list
>> >> > SG12_at_[hidden]
>> >> > Subscription: https://lists.isocpp.org/mailman/listinfo.cgi/sg12
>> >> > Link to this post: http://lists.isocpp.org/sg12/2020/04/0888.php