lightwalletd icon indicating copy to clipboard operation
lightwalletd copied to clipboard

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

Open defuse opened this issue 5 years ago • 3 comments

defuse avatar Dec 17 '19 20:12 defuse

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.

LarryRuane avatar Dec 18 '19 02:12 LarryRuane

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.

defuse avatar Dec 18 '19 17:12 defuse

@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.

LarryRuane avatar Nov 23 '22 23:11 LarryRuane