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

PyDoc2GitHub CWE-426 #566

Merged
merged 7 commits into from
Aug 7, 2024
Merged

PyDoc2GitHub CWE-426 #566

merged 7 commits into from
Aug 7, 2024

Conversation

tommcd
Copy link
Contributor

@tommcd tommcd commented Jul 29, 2024

Moving CWE-426 from confluence as part of #531

Note: existing folder XXX-005 should have been CWE-426 so it has been renamed.

@tommcd tommcd marked this pull request as ready for review July 29, 2024 15:30
@tommcd
Copy link
Contributor Author

tommcd commented Jul 29, 2024

Hi @gkunz,
I wanted to add Labels "Product: Python Hardening Guide" to this PR, but I don't see any way to do so.
The 'Labels' section on the right side of this page is not editable for me.
I assume I don't have the required permissions?

@gkunz
Copy link
Contributor

gkunz commented Jul 29, 2024

Hi @gkunz, I wanted to add Labels "Product: Python Hardening Guide" to this PR, but I don't see any way to do so. The 'Labels' section on the right side of this page is not editable for me. I assume I don't have the required permissions?

Hi @tommcd: yes, that seems to be the case. I just applied the label.

@gkunz
Copy link
Contributor

gkunz commented Aug 1, 2024

I tried to validate the example, but I don't fully understand the effect of the --check-hash-based-pycs flag. To my understanding, it should avoid some sort of cache poisoning, but I wasn't able to make the example work for me with regards to this option.

To be precise: the non-compliant example works as expected, but that's due to the -I flag.

Is my understanding correct that one should try to construct an example in which the cached file include different code than the original source? I tried to create two different cache files (one for the server, one for the print out) and replace one with the other, but that didn't work for me. Is that the intention?

Copy link
Contributor

@SecurityCRob SecurityCRob left a comment

Choose a reason for hiding this comment

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

thank you

@tommcd
Copy link
Contributor Author

tommcd commented Aug 7, 2024

I tried to validate the example, but I don't fully understand the effect of the --check-hash-based-pycs flag. To my understanding, it should avoid some sort of cache poisoning, but I wasn't able to make the example work for me with regards to this option.

To be precise: the non-compliant example works as expected, but that's due to the -I flag.

Is my understanding correct that one should try to construct an example in which the cached file include different code than the original source? I tried to create two different cache files (one for the server, one for the print out) and replace one with the other, but that didn't work for me. Is that the intention?

I believe you are correct. For now I reworded the description to make it clearer that -I address the exploit but --check-hash-based-pycs always adds additional security. I'll discuss with Helge to see if another or berrer example can be used.

@tommcd tommcd requested a review from gkunz August 7, 2024 14:02
@tommcd
Copy link
Contributor Author

tommcd commented Aug 7, 2024

Hi @gkunz,
I'm not sure how to fix the following:

"Changes requested
1 review requesting changes and 1 approving review by reviewers with write access.

Merging is blocked
Merging can be performed automatically once the requested changes are addressed."

As far as I can see the requested changes were already resolved?

@gkunz
Copy link
Contributor

gkunz commented Aug 7, 2024

Hi @gkunz, I'm not sure how to fix the following:

"Changes requested 1 review requesting changes and 1 approving review by reviewers with write access.

Merging is blocked Merging can be performed automatically once the requested changes are addressed."

As far as I can see the requested changes were already resolved?

@tommcd sorry, my bad. I'll update my review. Thanks for addressing my comments.

@gkunz gkunz merged commit 7158ce6 into ossf:main Aug 7, 2024
2 checks passed
@tommcd tommcd deleted the cwe426_531 branch August 8, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants