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

Add guidance on handling secrets in memory #1257

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Conversation

szh
Copy link
Collaborator

@szh szh commented Dec 8, 2023

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

🚩 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:

  • In case of a new Cheat Sheet, you have used the Cheat Sheet template.
  • All the markdown files do not raise any validation policy violation, see the policy.
  • All the markdown files follow these format rules.
  • All your assets are stored in the assets folder.
  • All the images used are in the PNG format.
  • Any references to websites have been formatted as [TEXT](URL)
  • You verified/tested the effectiveness of your contribution (e.g., the defensive code proposed is really an effective remediation? Please verify it works!).
  • The CI build of your PR pass, see the build status here.

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 😃

@szh szh added ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet. labels Dec 8, 2023
@szh szh linked an issue Dec 8, 2023 that may be closed by this pull request
Copy link
Member

@jmanico jmanico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jmanico jmanico merged commit 417a452 into master Dec 8, 2023
6 checks passed
@jmanico jmanico deleted the secrets-in-memory branch December 8, 2023 16:03
@kwwall
Copy link
Collaborator

kwwall commented Dec 8, 2023

I will add a review this evening.

@kwwall
Copy link
Collaborator

kwwall commented Dec 9, 2023

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
Copy link
Member

jmanico commented Dec 9, 2023 via email

@jmanico
Copy link
Member

jmanico commented Dec 9, 2023 via email

@kwwall
Copy link
Collaborator

kwwall commented Dec 9, 2023

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
Copy link
Member

jmanico commented Dec 9, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACK_OBTAINED Issue acknowledged from core team so work can be done to fix it. UPDATE_CS Issue about the update/refactoring of a existing cheat sheet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update: Secrets Management Cheat Sheet - String storage in memory
3 participants