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
Fixed a regular expression denial of service issue by limiting whites… #7360
Conversation
please review |
Seems like I've broken something. I'll see if I can fix it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test showing that more than 5 spaces is invalid with a comment as to why.
Otherwise LGTM.
please update |
won't other filler characters still cause the same issue? IMO it'd be better to just set a reasonable length limit like 2048 characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Awesome! @samuelcolvin can we open up a security advisor for this one? |
name_chars = r'[\w!#$%&\'*+\-/=?^_`{|}~]' | ||
unquoted_name_group = fr'((?:{name_chars}+\s+)*{name_chars}+)' | ||
quoted_name_group = r'"((?:[^"]|\")+)"' | ||
email_group = r'<\s*(.+)\s*>' | ||
email_group = r'<\s*(.{0,254})\s*>' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has changed the intent of the regex - previously it only matched one or more characters, now it matches 0 characters. Admittedly the previous regex would match a space alone if the address was < > and this will not, but it is a change beyond the statement in the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for anyone coming to this in future, in #7599 (before releasing this PR) we reverted this change so the only actual change implemented is the length check which covers the original minor performance risk.
See #7360 (comment). No regex changes were necessary in the end because we ended up just restricting the overal length
Will this change be backported to v1? |
yes we should, @hramezani please could you back port this to V1 and get a release out? |
Am I right that this fix is included in v1.10.13? |
@allanlewis Yes. it is. |
Excellent - thanks for arranging the backport in record time! |
@samuelcolvin would it be possible to open up a security advisory for this one? |
I really don't think it's necessary. |
…paces
Change Summary
As discussed over email, this fixes a security issue where a regular expression denial of service can be performed.
Exploit code is below:
Related issue number
NA
Checklist
Selected Reviewer: @samuelcolvin