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

7.2.4 (v4.0.3-9.2.5) - improve the wording to cover all connection errors and failed certificate checks #1902

Closed
elarlang opened this issue Mar 17, 2024 · 10 comments
Assignees
Labels
7) PR in non-master branch V7 Temporary label for grouping logging related issues _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

Current requirement was moved from 9.2.5 to 7.2.4 (#1890)

# Description L1 L2 L3 CWE
7.2.4 [MOVED FROM 9.2.5] Verify that backend TLS connection failures are logged. 778

Proposal: make the requirement to cover all connection errors.

.. or, have 2 requirements:

  • all connection errors (database connection, external API connection, etc)
  • all failed certificate checks
@elarlang elarlang added the V7 Temporary label for grouping logging related issues label Mar 17, 2024
@tghosth tghosth added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Mar 17, 2024
@elarlang elarlang added the next meeting Filter for leaders label Mar 26, 2024
@tghosth
Copy link
Collaborator

tghosth commented Apr 7, 2024

I still don't like this. I think it is too specific and for logging we need to focus on principles rather than specific events. I would happily just drop this requirement.

I also think we need to better differentiate between regular logging (which might be a short term thing) and security audit logs which may need to be longer term (e.g. account details change history).

@elarlang
Copy link
Collaborator Author

elarlang commented Apr 7, 2024

My proposal was to make it more abstract.

Anyway, waiting outcome from #1795 (comment)

@elarlang elarlang added the 4a) Waiting for another This issue is waiting for another issue to be resolved label Apr 7, 2024
@jmanico
Copy link
Member

jmanico commented Apr 8, 2024

Elar. As mentioned in #1795 (comment) I think it's fair to see security logging as a lower priority in the interest of getting 5.0 live.

Perhaps a more general requirement "do security logging on ASVS requirements that fail" or similar is the right way to go for 5.0.

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

This should be removed once #1944 is merged. The original requirement should be tagged as merged into 7.2.3.

Additional events added to the logging cheatsheet here: OWASP/CheatSheetSeries#1394

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

@set-reminder in 1 week @tghosth to address once #1944 is merged

Copy link

octo-reminder bot commented May 2, 2024

Reminder
Thursday, May 9, 2024 12:00 AM (GMT+02:00)

in @tghosth to address once #1944 is merged

@tghosth tghosth removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet next meeting Filter for leaders labels May 2, 2024
@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

@elarlang actually I see this as slightly different case to 7.2.3.

7.2.3 is deliberate bypass attempts. 7.2.6 is general security control failures, might not be malicious. I would therefore keep them separate.

Opened #1947

What do you think?

@tghosth tghosth added 6) PR awaiting review and removed 4a) Waiting for another This issue is waiting for another issue to be resolved labels May 2, 2024
@elarlang
Copy link
Collaborator Author

elarlang commented May 2, 2024

I can understand the difference, but I'm concerned, is it also understandable when just reading the requirements.

# Description L1 L2 L3 CWE
7.2.3 [MODIFIED, MOVED FROM 7.1.3] Verify that the application logs attempts to bypass the security controls defined in the design documentation such as input validation. 778
7.2.6 [MOVED FROM 9.2.5] Verify that the application logs security control failures such as backend TLS failures. 778

I don't have any recommendations, accepting PR.

@tghosth
Copy link
Collaborator

tghosth commented May 2, 2024

Let's leave it for now and see if anyone complains during the draft :)

Copy link

octo-reminder bot commented May 8, 2024

🔔 @tghosth

in @tghosth to address once #1944 is merged

elarlang pushed a commit to elarlang/ASVS that referenced this issue May 9, 2024
jmanico pushed a commit that referenced this issue May 9, 2024
* label correction for 13.1.1 + 5.5.5, #1538

* label correction for 11.1.7, 11.1.8 #1272

* label correction for 7.2.6 #1890, #1902

* label correction for 13.1.1 + 5.5.5, #1538

---------

Co-authored-by: Elar Lang <elar@hoh.ee>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7) PR in non-master branch V7 Temporary label for grouping logging related issues _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

3 participants