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

Avoid DoS on carefully crafted spec files (fix #61) #62

Closed
wants to merge 1 commit into from

Conversation

kraptor
Copy link
Contributor

@kraptor kraptor commented Sep 12, 2023

Fixes #61 by using a maximum number of attempts while replacing macros recursively.

Copy link

@mcepl mcepl left a comment

Choose a reason for hiding this comment

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

Yes, I think, this is correct. Thank you.

@kraptor
Copy link
Contributor Author

kraptor commented Sep 18, 2023

Added the missing newline so the linter doesn't complain.

Copy link

@mcepl mcepl left a comment

Choose a reason for hiding this comment

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

Still good.

@bkircher
Copy link
Owner

Hey. Thanks for the PR. Is there a reason for max_attempts being 1000? And why make it an argument at all?

@bkircher
Copy link
Owner

Is it OK with you guys if we make max_attempts local to that function?

@kraptor
Copy link
Contributor Author

kraptor commented Sep 25, 2023

Is it OK with you guys if we make max_attempts local to that function?

I left the max_attempts as a param to keep compatibility and/or let the user to call that function to whatever value seems/works appropriate for them

The use of 1000 is just an arbitrary number, we can set it to any value that is high enough so the while loop ends.

Finally @bkircher. maybe it's better to just throw an exception instead of returning when that max_attempts is reached. To me, as far as the loop exits in some way is an acceptable solution instead of looping forever.

Just let me know what you want and I'll update the PR.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@bkircher
Copy link
Owner

I left the max_attempts as a param to keep compatibility and/or let the user to call that function to whatever value seems/works appropriate for them

OK for me.

The use of 1000 is just an arbitrary number, we can set it to any value that is high enough so the while loop ends.

OK.

Finally @bkircher. maybe it's better to just throw an exception instead of returning when that max_attempts is reached. To me, as far as the loop exits in some way is an acceptable solution instead of looping forever.

Good one. I cherry-picked this into cfbde15 and added a subsequent commit that raises a RuntimeError instead of returning from that function.

@bkircher bkircher closed this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DoS with carefully crafted spec files
3 participants