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

1.6.4 requirement is too complicated to understand #685

Closed
belalena opened this issue Oct 10, 2019 · 29 comments
Closed

1.6.4 requirement is too complicated to understand #685

belalena opened this issue Oct 10, 2019 · 29 comments
Assignees
Labels
6) PR awaiting review V6 _5.0 - prep This needs to be addressed to prepare 5.0
Milestone

Comments

@belalena
Copy link

1.6.4 Verify that symmetric keys, passwords, or API secrets generated by or shared with clients are used only in protecting low risk secrets, such as encrypting local storage, or temporary ephemeral uses such as parameter obfuscation. Sharing secrets with clients is clear-text equivalent and architecturally should be treated as such.

I'm not a native english speaker and it's completely not clear for me what does it mean:

  • "are used only in protecting low risk secrets" - symmetric keys, passwords and so on are used to protect low risk secrets ( such as ..)? and what about high risk secrets?
    OR the meaning is "are used in protected low risk way (such as ...)";

  • "temporary ephemeral uses such as parameter obfuscation" - temporary = parameter obfuscation? OR parameter obfuscation should be temporary measure?

"Sharing secrets with clients is clear-text equivalent and architecturally should be treated as such." - what is "as such"?

**why not to form it smth like

Verify that symmetric keys, passwords, API secrets, keystorages, certificates and other secrets shared with clients are used in protected low risk way (for example, they are stored in encrypted storage on the client side, they are temporary, they are obfuscated)..**

@m8urnett
Copy link
Contributor

Is this wording more clear?

Verify that the architecture treats client-side secrets--such as symmetric keys, passwords, or API tokens--as insecure and never uses them to protect or access sensitive data.

@belalena
Copy link
Author

Is this wording more clear?

Verify that the architecture treats client-side secrets--such as symmetric keys, passwords, or API tokens--as insecure and never uses them to protect or access sensitive data.

Sounds much easier to understand. Thank you. BUT if the meaning is as you wrote then I didn't catch what to use instead of symmetric keys, passwords, or API tokens to access sensitive data ? Can you please point me to req. about it or so?

@tghosth
Copy link
Collaborator

tghosth commented Apr 29, 2020

Hi @belalena The point is that symmetric secrets which the client has access to should not be used to protect sensitive data. Instead, a secret which is not shared with the client should be used to protect sensitive data.

Does that make more sense?

@tghosth tghosth added the 2) Awaiting response Awaiting a response from the original poster label Apr 29, 2020
@vanderaj vanderaj self-assigned this Jun 23, 2020
@vanderaj vanderaj added this to the 4.0.2 milestone Jun 23, 2020
@vanderaj
Copy link
Member

This makes sense as an inclusion to 4.0.2

@vanderaj vanderaj assigned jmanico and unassigned vanderaj Jul 12, 2020
jmanico added a commit that referenced this issue Jul 13, 2020
@jmanico
Copy link
Member

jmanico commented Jul 13, 2020

Change submitted 80498cf

@jmanico jmanico closed this as completed Jul 13, 2020
@aspnetde
Copy link

aspnetde commented Feb 8, 2023

Hi together,

I just stumbled across that item as it made its way into version 4.0.3 of the ASVS:

1.6.4: Verify that the architecture treats client-side secrets–such as symmetric keys, passwords, or API tokens–as insecure and never uses them to protect or access sensitive data.

I see a lot of scenarios where this makes absolutely sense, e.g., keys of third-party APIs which should not be shared with the client under any circumstances. However, there are also scenarios where this phrasing appears to be a little too broad/generic. E.g., consider a web application which implements end-to-end encryption with a zero-knowledge policy for the server and the client app acting as "ends". In that case the web app needs to deal with keying material etc. by definition. There's a lot of arguments that can be made against such a scenario in the first place, of course, but it's nothing which is inherently bad or wrong.

So I'd suggest to make that item a bit more precise.

@tghosth

@elarlang
Copy link
Collaborator

elarlang commented Feb 8, 2023

I reopen it for @aspnetde

In general - comments in already closed old (in this case 2years+) issues may not get noticed and it makes sense to open new issue

@tghosth
Copy link
Collaborator

tghosth commented May 23, 2023

Hi @aspnetde

This is the current wording, how would you suggest refining it?

# Description L1 L2 L3 CWE
1.6.4 [GRAMMAR] Verify that the architecture treats client-side secrets, (such as symmetric keys, passwords, or API tokens,) as insecure and never uses them to protect or access sensitive data. 320

@tghosth tghosth added 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos _5.0 - prep This needs to be addressed to prepare 5.0 and removed 2) Awaiting response Awaiting a response from the original poster labels May 23, 2023
@aspnetde
Copy link

@tghosth I am not familiar with the general rules here and it certainly is a hard nut to crack. I'd probably add an exception to the rule:

Verify that the architecture treats client-side secrets (such as symmetric keys, passwords, or API tokens) as insecure and never uses them to protect or access sensitive data except for scenarios like end-to-end encryption where the client acts as an end.

@tghosth
Copy link
Collaborator

tghosth commented Jun 5, 2023

Thanks @aspnetde

Do you think this is relevant for a web standard? For a desktop or mobile app I can imagine long term storage of keys but I think that is less likely in a browser context which would be in scope for us.

@aspnetde
Copy link

aspnetde commented Jun 5, 2023

I absolutely think that this is relevant. I mean, there's the Web Cryptography API, isn't it? The current phrasing is basically in conflict with any kind of usage of it. And there's a lot of useful scenarios which can and will be covered by web applications, regardless of keying material is kept short-term or long-term.

@tghosth
Copy link
Collaborator

tghosth commented Jun 5, 2023

Ok so I think the long-term concept is what is problematic for me here. I can see the use of client-side secrets for short-term/temp secrets used for transmission but I think it is the long-term of encryption at rest thing which is problematic. Do you think we can be more specific about how we narrow this down?

@aspnetde
Copy link

aspnetde commented Jun 5, 2023

The whole point of E2EE is to have data encrypted at rest and not just during transmission. The browser application can act as an "end" here, as can any desktop and mobile app. There are a lot of things to consider, e.g., clearing keying material on session-end. But that doesn't fit into a single paragraph.

If this doesn't comply with your view on the matter, I am fine with it. I do not need to comply with the checklist in any way.

@jmanico
Copy link
Member

jmanico commented Jun 5, 2023 via email

@tghosth
Copy link
Collaborator

tghosth commented Jun 7, 2023

The whole point of E2EE is to have data encrypted at rest and not just during transmission. The browser application can act as an "end" here, as can any desktop and mobile app. There are a lot of things to consider, e.g., clearing keying material on session-end. But that doesn't fit into a single paragraph.

Yeah I agree it is a little more complicated. For example, in any normal E2EE scheme, any secret keys which are stored on the client are not going to be used to protect data at rest on the server. I think maybe that it is the point here that keys that are on the client cannot be used to protect something anywhere else...

@tghosth
Copy link
Collaborator

tghosth commented Jul 10, 2023

Hi @aspnetde,

what do you think about the following:

# Description L1 L2 L3 CWE
1.6.4 [GRAMMAR] Verify that the architecture treats client-side secrets, (such as symmetric keys, passwords, or API tokens,) as insecure and never uses them to protect sensitive data being stored at the application back-end. 320

@tghosth tghosth added _5.0 - Not blocker This issue does not block 5.0 so if it gets addressed then great, if not then fine. 2) Awaiting response Awaiting a response from the original poster and removed _5.0 - prep This needs to be addressed to prepare 5.0 4b Major-rework These issues need to be part of a full chapter rework labels Nov 7, 2024
@jmanico
Copy link
Member

jmanico commented Nov 7, 2024

+1 Josh

@aspnetde
Copy link

aspnetde commented Nov 8, 2024

What would "further protection" be here?

@tghosth
Copy link
Collaborator

tghosth commented Nov 10, 2024

I guess my thought was that I understand the E2EE point but my assumption is that if data is sensitive enough to be E2EE then it will also be stored at the server side with another layer of encryption so that even if data leaks unintentionally from the server side, the client side key can't be used to decrypt it.

@aspnetde

@aspnetde
Copy link

I'm actually surprised how these formulations came about here. In my view, what you're writing here is quite subjective. I've already explained before how E2EE can be implemented using the Web Cryptography API. Regarding the keying material, it's solid from my perspective because the keys can be created and managed in such a way that they cannot be intercepted client-side other than they could on the server side. If there are problems, they relate to key rotation etc. - but that has not much to do with the core question here.

However, I don't have any increased interest in haggling over individual words here. Do as you think best. Good luck.

@tghosth tghosth added the next meeting Filter for leaders label Nov 11, 2024
@tghosth
Copy link
Collaborator

tghosth commented Nov 14, 2024

Maybe needs to be "documentation" regarding where you are using keys and where they can be exposed.

@tghosth
Copy link
Collaborator

tghosth commented Nov 14, 2024

@set-reminder @tghosth to have another look in 2 days

Copy link

octo-reminder bot commented Nov 14, 2024

Reminder
Saturday, November 16, 2024 12:00 AM (GMT+01:00)

@tghosth to have another look in

@tghosth tghosth removed the next meeting Filter for leaders label Nov 14, 2024
Copy link

octo-reminder bot commented Nov 15, 2024

🔔 @tghosth

@tghosth to have another look in

@tghosth tghosth added 4) proposal for review Issue contains clear proposal for add/change something _5.0 - prep This needs to be addressed to prepare 5.0 and removed 2) Awaiting response Awaiting a response from the original poster _5.0 - Not blocker This issue does not block 5.0 so if it gets addressed then great, if not then fine. labels Nov 18, 2024
@tghosth
Copy link
Collaborator

tghosth commented Nov 18, 2024

So stuff happened and we currently have the following:

# Description L1 L2 L3 CWE
1.6.4 [GRAMMAR] Verify that the architecture treats client-side secrets (such as symmetric keys, passwords, or API tokens) as insecure and never uses them to protect or access sensitive data. 320
1.6.5 [ADDED] Verify that a cryptographic inventory is performed, maintained, regularly updated, and includes all cryptographic keys, algorithms, and certificates used by the application. 311
1.6.7 [ADDED] Verify that cryptographic discovery mechanisms are employed to identify all instances of cryptography in the system, including encryption, hashing, and signing operations. 311

The discussion in this issue demonstrates to me that client side encryption (1.6.4) is not a "one size fits all" situation and seems to fit well with our "security decision concept" which 1.6.5 and 1.6.7 (sic) also fit into.

As such, I would suggest the following:

# Description L1 L2 L3 CWE
1.6.4 [MODIFIED] Verify that a cryptographic inventory is performed, maintained, regularly updated, and includes all cryptographic keys, algorithms, and certificates used by the application. It should also document where keys can and cannot be used in the system and also the types of data which can and cannot be protected using the keys. 320
1.6.5 [ADDED] Verify that cryptographic discovery mechanisms are employed to identify all instances of cryptography in the system, including encryption, hashing, and signing operations. 320

(Notice the level and CWE changes as well)

I think this resolves this current issue and also clarifies the scope of the cryptographic inventory.

However, I think this needs to wait until #2371 gets merged.

@tghosth tghosth added the 4a) Waiting for another This issue is waiting for another issue to be resolved label Nov 18, 2024
@randomstuff
Copy link
Contributor

randomstuff commented Nov 18, 2024

I don't understand 1.6.5. What is a "cryptographic discovery mechanism"?

@tghosth
Copy link
Collaborator

tghosth commented Nov 19, 2024

I don't understand 1.6.5. What is a "cryptographic discovery mechanism"?

Maybe open a separate issue for that @randomstuff

@tghosth tghosth added 6) PR awaiting review and removed 4) proposal for review Issue contains clear proposal for add/change something 4a) Waiting for another This issue is waiting for another issue to be resolved labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review V6 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

9 participants