OWASP / CheatSheetSeries

The OWASP Cheat Sheet Series was created to provide a concise collection of high value information on specific application security topics.
https://cheatsheetseries.owasp.org
Creative Commons Attribution Share Alike 4.0 International
27.75k stars 3.89k forks source link

Add guidance on handling secrets in memory #1257

Closed szh closed 10 months ago

szh commented 10 months ago

Thank you for submitting a Pull Request (PR) to the Cheat Sheet Series.

:triangular_flag_on_post: If your PR is related to grammar/typo mistakes, please double-check the file for other mistakes in order to fix all the issues in the current cheat sheet.

Please make sure that for your contribution:

If your PR is related to an issue, please finish your PR text with the following line:

This PR covers issue #1223

Thank you again for your contribution :smiley:

kwwall commented 10 months ago

I will add a review this evening.

kwwall commented 10 months ago

On 12/8/2023 at 11:03am ET, @jmanico, you wrote:

This is very well written and explains the low risk but gives good guidance.

and then proceeded to merge this. Exactly 1 hour prior to that, I got received an email notice in my Gmail inbox inbox_screenshot_everything,everywhere,all-at-oncerequesting I approve this PR. During that time, I was working and was unable to respond.

Now, I don't necessarily disagree with your assessment and in general, you know that I generally am pragmatic and not one to let the perfect become the enemy of the good, but in this case I do wish you would have waited to for me to review this, as I did have some comments that I wanted to make. However, on a broader level, I will say if their are N people who were asked to review a PR, we should at least wait 50% or more of them weigh in or else specific note that they want to decline in commenting / approving. In ESAPI we have its repo configured to require at least 2 approvers for those are are not the code owners (myself, Matt, and Jeremiah) and at least 1 approver if any of the "code owners" submits a PR. It may be prudent to do something similar here.

Getting back to my making further comments on this closed PR, given that you merged it and then deleted the branch, I'm not even certain that I can make any additional comments on the MD file. I could of course create another issue on this topic and a 2nd PR, but that seems like an excessive amount of effort for so little updated content. So instead, let me try to describe what I wanted to state and if you agree and @mackowski agree, then perhaps I shall be persuaded to proceed doing that.

What I wanted to comment on was this statement:

However, this approach may be considered overkill in many cases, as it is only useful if the attacker already has access to the memory of the process handling the secret. By that point, the security breach may have already occurred.

While this is technically correct, I think it misses the whole threat modeling point that I was trying to make. The general assumption is of course that for most modern operating systems, different processes executing as different user IDs will generally not be able to access each other's memory space as either the OS itself or the MMU hardware will prevent that. However, dating back to 2014, first Rowhammer, and then a few years later the Meltdown and Spectre attacks show us that these assumptions just generally were not true. One does not generally consider this a case of "an adversary already having access to the memory of the process handling the secret." although this is technically what it implies. However, writing an explicit threat model for ones application ought to surface this as a concern and only at that point can you make a conscious rationale decision whether you wish to accept the risk or try to mitigate it. I don't thin that's splitting hairs. In some cases, you legitimately may not care because the secret you are trying to protect is some where the assets have such a low value that it would be exorbitant to spend all those development dollars to provide an effective mitigating control. Other cases you make find that the stakes are so high if an adversary discovered your secret it could mean significant unacceptable financial loss for your company. Even then, the answer might not be "overwrite the secret" (which can be very difficult to implement for garbage collected memory), but rather to do something like "deploy on a private cloud" instead of a public cloud. I think if you haven;t at least thought through a threat model for this and understand who the potential threat actors are, you won't be able to provide a credible answer to this question.

Anyhow, that is all that I have to see about the PR. I just would have liked the 3 quoted sentences cleared up a bit in regards to stating developers common intuition about memory safety is mostly wrong and that we really need to do threat modeling to help us understand the underlying risks and what to do to address them.

jmanico commented 10 months ago

My apologies. We can. Reverse this out.

jmanico commented 10 months ago

I hear you, can I see your PR? I’ll take your advice here Kevin even if you wish to remove it.

kwwall commented 10 months ago

Jim, I am currently busy working with Mark Gamache on updating the ancient "Certificate and Public Key Pinning" OWASP wiki page which is a major rewrite.(And Mark is doing a great job leading.) If I get a few spare cycles, I will give a PR a crack, but would prefer hear what @mackowski thinks about my comments before I proceed with doing that. I already have too many OWASP irons in the fire and with the holidays rapidly approaching, some additional family obligations at all.

Also, my apologies if I came off as harsh or critical of you. I know you have the best intentions in mind and I do think this was rather well-written. Thanks for your understanding in this matter.

jmanico commented 10 months ago

If I could represent my love for you Kevin, I’d put it in a variable that was encrypted or protected in some other way so no one could steal it from memory.

That’s how much I care, even when we fight.

❤️