Skip to content

Comments

Make it more clear to auditors that security-critical regexp can't match strings with newlines#4

Open
defuse wants to merge 1 commit intoadityapk00:masterfrom
defuse:fix-regexp
Open

Make it more clear to auditors that security-critical regexp can't match strings with newlines#4
defuse wants to merge 1 commit intoadityapk00:masterfrom
defuse:fix-regexp

Conversation

@defuse
Copy link

@defuse defuse commented Dec 17, 2019

No description provided.

@LarryRuane
Copy link

Although the suggested change is okay, my experiments show this is not a problem. The Golang regexp documentation is here: https://github.com/google/re2/wiki/Syntax. I wrote a small test program to try to reproduce the problem (you can click on Run to run the program):

https://play.golang.org/p/NrgjszRg-9g

As you can see, the pattern doesn't match (match is false). On further investigation, I found that the documentation says that ^ and $ match the beginning and end of any line (within a possibly much longer string) only if "multi-line" mode is enabled (search within that page for "multi-line"). If we modify the test program to enable multi-line mode (the pattern string begins with (?m), which is how the multi-line mode flag is specified):

https://play.golang.org/p/edqhoeeVqon

As you can see if you run this program, the pattern now matches. Here is that program, in case the above playground link stops working:

package main

import (
	"fmt"
	"regexp"
)

func main() {
	match, err := regexp.Match("(?m)^t[a-zA-Z0-9]{3}$", []byte("taaa\nthis is a separate line"))
	fmt.Println(match, err)
}

Because our code doesn't specify the multi-line flag, I think the pattern matching as written (with ^ and $) works as desired.

@defuse defuse changed the title Bug fix: strings with newlines could circumvent t-address validity check Make it more clear to auditors that security-critical regexp can't match strings with newlines Dec 18, 2019
@defuse
Copy link
Author

defuse commented Dec 18, 2019

Thanks @LarryRuane! I think it's worth using \A and \z so it's obvious there's no problem, I reworded the commit message to indicate that.

@LarryRuane
Copy link

@defuse, sorry, I forgot about this. Do you still think it's worth doing? If so, you could reopen it and I could quickly merge it.

@defuse defuse reopened this Nov 29, 2022
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.

2 participants