Documentation: Mention pinning on the Sensitive data page

[samuel-lucas6]

I think it's worth mentioning that sensitive byte arrays/spans should be pinned for their lifetime to avoid copies in memory. There are several ways to do this:

1. stackalloc expression
2. GC.AllocateArray()
3. fixed statement
4. GCHandle.Alloc()

Otherwise, CryptographicOperations.ZeroMemory() can be useless and give you a false sense of security.

It may also be worth explicitly stating that strings should be avoided if possible because they supposedly can't be wiped without a risk of crashing the runtime, although it seems to be doable in a project I'm working on that calls Monocypher.

Overall though, this library has pretty good documentation, so thank you for that.

// MAINTAINER EDIT Adding links to the specific documentation article the OP is referring to: https://github.com/Yubico/../docs/../sensitive-data.md

[DennisDyallo]

Hi @samuel-lucas6 ! Thanks for the comment. I know this topic has been discussed previously in the team prior to me joining. I'll bring it to the team and see what comes up.

[DennisDyallo]

Hi @samuel-lucas6!

I want to start off by saying that I'm new to the team and Yubico. But based on the discussions regarding your issue/suggestion I can reply that we generally agree with your suggestions. The documentation page should be rewritten to address the corncerns you're raising. At the same time, in the topic of "false sense of security", we acknowledge that if an exploiter has access to a memory dump, there are likely bigger problems in your organization than Yubikey pin codes being leaked from memory.

Feel free to add to this discussion.

//Dennis

[samuel-lucas6]

Thanks for discussing it. I agree, but it's considered good practice and better than doing nothing. It's probably more about preventing secrets getting stored on disk than an attacker performing a memory dump, although the latter has been discussed in the context of password managers.

The only trouble is that the library isn't designed for this. For example, I wrote a program supporting challenge-response and noticed that requires using ReadOnlyMemory<byte> for the response.

[DennisDyallo]

Thanks for the issue @samuel-lucas6 We posted an update to the referred article. This version adresses the concerns you raised. Please take a look and let us know what you think.

[samuel-lucas6]

Hey @DennisDyallo, thanks for updating the documentation and letting me know.

I have four pieces of feedback:

1. I think slicing spans should be mentioned to avoid copies. I believe that was there before.
2. The examples should pin the byte array (e.g., switch to using a span with stackalloc or do GC.AllocateArray()).
3. The fact that strings are problematic is discussed twice. You could probably get rid of it from the first numbered bullet point and merge bullets 1 and 5.
4. I think the previous conclusion was clearer about how there's nothing else you can do (but C# is better than some languages) and that sensitive data not ending up on disk is virtually impossible to prevent. Maybe disk encryption could be mentioned as a mitigation if this is a concern.

Otherwise, I like the bullet point approach and discussion of the different risks.