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

relation between ASVS-v4.0.3 7.2.1, 7.1.2.2 and 7.1.3, 7.1.4 #997

Closed
timhemel opened this issue May 16, 2021 · 26 comments · Fixed by #1794
Closed

relation between ASVS-v4.0.3 7.2.1, 7.1.2.2 and 7.1.3, 7.1.4 #997

timhemel opened this issue May 16, 2021 · 26 comments · Fixed by #1794
Assignees
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review Community wanted We would like feedback from the community to guide our decision otherwise we will progress V7 Temporary label for grouping logging related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@timhemel
Copy link

timhemel commented May 16, 2021

Reading 7.1 and 7.2 it seemed to me that there may be some duplication. Perhaps I have missed something.

It looks to me as if 7.2.1 and 7.2.2 are specific cases of 7.1.3 and 7.1.4 combined, and if so, they would be redundant. The descriptions in the sections don't really help me to understand the difference. One section seems to be about the content, the other about the format. Could someone explain this?

| 7.1.3 | Verify that the application logs security relevant events including successful and failed authentication events, access control failures, deserialization failures and input validation failures. (C5, C7) | | ✓ | ✓ | 778 |
| 7.1.4 | Verify that each log event includes necessary information that would allow for a detailed investigation of the timeline when an event happens. (C9) | | ✓ | ✓ | 778 |
| 7.2.1 | Verify that all authentication decisions are logged, without storing sensitive session tokens or passwords. This should include requests with relevant metadata needed for security investigations. | | ✓ | ✓ | 778 |
| 7.2.2 | Verify that all access control decisions can be logged and all failed decisions are logged. This should include requests with relevant metadata needed for security investigations. | | ✓ | ✓ | 285 |

If I explain logging to developers, I tell them they need to decide which events to log (i.e. all relevant security events, failed and successful) and what data to log for that event (enough for a security investigation, but as few sensitive data as possible). 7.1 nicely summarizes this.

@elarlang
Copy link
Collaborator

elarlang commented May 16, 2021

another one from my todo list :)

My current improvement idea on that field:
7.1 should be clearly "what must every log line contain and what it should not contain", "Log Content" is ok category name here. I'm ok with this part.
7.2 is the confusing one here. It seems to be list of security events. Category name "Log Processing" is confusing here. I think I have discussed it in some issue already, but did not find it at the moment. And my current setup, I agree, both 7.2.1 and 7.2.2 are covered by 7.1.3+7.1.4.

Good material was recently published under CheatSheet project:
https://cheatsheetseries.owasp.org/cheatsheets/Application_Logging_Vocabulary_Cheat_Sheet.html

Now it's good question, how to list all expected security events - at the moment there is only 2: authentication and authorization but this gives a false signal - like logging only those is already good enough.

@timhemel
Copy link
Author

timhemel commented May 16, 2021

Sorry, I did not know it was on your list already :) . I tried to my best to search if the issue was reported already but could not find it.

Do I understand correctly that you intend 7.1 to be about the 'what' and 7.2 about the 'when'? Currently both are in 7.1 and that is fine, i don't see any reason to split it, unless you want to be more detailed about the specific events.

It is going to be hard to be complete with respect to expected security events, as some events may be very domain specific. Therefore I would use the system's threat model as inspiration. Good candidates would be:

  • any functionality that requires non-repudiation
  • any important status of a mitigation in progress, such as the result of a security check or a security decision.

Of course the ASVS could suggest some common ones, like authentication, authorization, validation, DoS-protection, security relevant errors, attack detection. That list in the cheatsheet is a good source of inspiration, it could grow into a standard of its own!

So, I would give the reader the general guidelines and some examples, rather than give the false impression of completeness.

@elarlang
Copy link
Collaborator

elarlang commented May 17, 2021

Sorry, I did not know it was on your list already :) . I tried to my best to search if the issue was reported already but could not find it.

No reason to say sorry. I like that someone else notices this kind stuff as well. And I like your fact-based arguments, keep going!

It is going to be hard to be complete with respect to expected security events, as some events may be very domain spec

One way to go is to have "architecture" requirement like (however to say it in English correctly) "Verify, that all expected security events for logs are documented." Like requirement #892

This could be required input for current 7.1.3. In this case 7.2.* requirements are not necessary, but on the other hand, it's nice to point out "minimum list of events what must be logged".

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Jul 14, 2021
@drwetter
Copy link

drwetter commented Aug 5, 2021

Either we take out "including successful and failed authentication events, access control failures," from 7.1.3 and leave this to in 7.2.1 + 7.2.2 . Or better: change the order. To me it is more relevant to log failed login attempts and similar (thus it maybe it better to put important things first but that doesn't seem relevant here). Also please Level 1 for auth events, see #1040.

@elarlang
Copy link
Collaborator

elarlang commented Aug 5, 2021

Logging and level 1 - by current description (see https://github.com/OWASP/ASVS/blob/master/4.0/en/0x03-Using-ASVS.md), level 1 is not the case for logging related requirements, as those require access to logs.

@drwetter
Copy link

drwetter commented Aug 5, 2021

Right, forgot that definition (if you mean completely penetration testable and not Level 1 is the bare minimum that all applications should strive for).

OT: It should be either one or the other

@elarlang
Copy link
Collaborator

elarlang commented Aug 5, 2021

and on this topic there is discussion in #956

@drwetter
Copy link

drwetter commented Aug 5, 2021

yeah maybe and thx for the hint. But participate in that discussion is too much for me.

My personal stake is that at least ASVS should be more clear and not on one side completely penetration testable and on the other hand not Level 1 is the bare minimum that all applications should strive for . That seems a stretch to me and that needs to be fixed - YMMV

@tghosth
Copy link
Collaborator

tghosth commented Feb 23, 2022

@elarlang any idea what the next action on this is?

@elarlang
Copy link
Collaborator

Starting point here is - how do we cover "all security events what must be logged" without listing them.

@jmanico
Copy link
Member

jmanico commented Apr 20, 2022 via email

@elarlang
Copy link
Collaborator

May I suggest we take the OWASP logging vocabulary cheat sheet and build a separate logging standard for logging events? https://cheatsheetseries.owasp.org/cheatsheets/Logging_Vocabulary_Cheat_Sheet.html

this is something to do, but do not answer to my question at all - how to cover it in ASVS :)

@jmanico
Copy link
Member

jmanico commented Apr 20, 2022

It does answer your question, let me help.

By suggesting we move the logging requirements to a separate standard, I suggest that we do NOT provide those details in ASVS.

Is that suggestion helpful?

@tghosth
Copy link
Collaborator

tghosth commented Apr 26, 2022

I think we need to provide high level requirement in ASVS and in the references section link to the relevant cheatsheet. So what is the next action here @elarlang ?

@jmanico
Copy link
Member

jmanico commented Sep 28, 2022

I'd like to add that when looking at:

7.1.3 and 7.2.1 both (in part) contain the same information.

https://github.com/OWASP/ASVS/blob/master/5.0/en/0x15-V7-Error-Logging.md

@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 josh/elar and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Dec 7, 2022
@elarlang
Copy link
Collaborator

elarlang commented Nov 26, 2023

ping @timhemel - I did some reorganizing and deduplications #1794

It all requires more work, but lets start from the first step - do you think it solves the first set of problems addressed when the issue was opened (#997 (comment))?

@elarlang
Copy link
Collaborator

The scope for this issue: duplication between requirements 7.1.3 (proposed to move to 7.2.3), 7.1.4, 7.2.1, 7.2.2

For covering different security events, I opened separate issue: #1795 which also should handle potential need to modify or finetune requirements 7.1.3/7.2.3, 7.2.1 and 7.2.2.

@tghosth
Copy link
Collaborator

tghosth commented Nov 28, 2023

hi @elarlang, in #1794

a) We lose some detail from 7.2.1 and 7.2.2. "This should include requests with relevant metadata needed for security investigations". Are you sure this is covered elsewhere already?

b) Why did you comment out the surrounding text?

@elarlang
Copy link
Collaborator

elarlang commented Nov 28, 2023

I tried to put self-explaining commit messages:
#1794

a) commit 5790450 says "deduplicate 7.2.1 and 7.2.2 from 7.1.4"

and current 7.1.4 is:

# Description L1 L2 L3 CWE
7.1.4 Verify that each log event includes necessary information that would allow for a detailed investigation of the timeline when an event happens. (C9) 778

b) commit 1584d72 says "hide outdated section descriptions"

Those texts are outdated - if we make changes and want to get feedback, we don't want to get feedback for already known outdated or out of sync intro texts.

@tghosth
Copy link
Collaborator

tghosth commented Nov 28, 2023

Ah ok, so what do you think about rewording 7.1.4 slightly:

# Description L1 L2 L3 CWE
7.1.4 [MODIFIED] Verify that each log entry includes necessary metadata that would allow for a detailed investigation of the timeline when an event happens. (C9) 778

@elarlang
Copy link
Collaborator

I don't mind, but I can not see improvement from "information" > "metadata" change. Any reasoning?

@elarlang elarlang added next meeting Filter for leaders 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Nov 29, 2023
elarlang pushed a commit to elarlang/ASVS that referenced this issue Nov 30, 2023
@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR next meeting Filter for leaders labels Nov 30, 2023
@elarlang
Copy link
Collaborator

@tghosth - PR updated via elarlang@b43f8b7

@elarlang elarlang changed the title relation between ASVS-4.0-7.2.{1,2} and 7.1.{3,4} relation between ASVS-v4.0.3 7.2.1, 7.1.2.2 and 7.1.3, 7.1.4 Dec 1, 2023
tghosth pushed a commit that referenced this issue Dec 1, 2023
tghosth pushed a commit that referenced this issue Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review Community wanted We would like feedback from the community to guide our decision otherwise we will progress V7 Temporary label for grouping logging related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
5 participants