gortr icon indicating copy to clipboard operation
gortr copied to clipboard

Panic with invalid SLURM file

Open agallo opened this issue 5 years ago • 5 comments

I did some testing and found that goRTR either panics or has undesirable behavior if invalid data is provided. Maybe do some sanity checking before accepting data?

When an invalid prefix (v4 or v6, either the address portion or mask portion) is provided in the prefix assertion section, goRTR panics with:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x6af31e]

goroutine 1 [running]:
github.com/cloudflare/gortr/prefixfile.(*SlurmLocallyAddedAssertions).AssertROAs(0xc00014a460, 0xc009b72000, 0x24de6, 0x32a16)
        /home/agallo/go/src/github.com/cloudflare/gortr/prefixfile/slurm.go:138 +0x12e
github.com/cloudflare/gortr/prefixfile.(*SlurmConfig).AssertROAs(...)
        /home/agallo/go/src/github.com/cloudflare/gortr/prefixfile/slurm.go:154
main.(*state).updateFile(0xc0000c0000, 0x7ffc1078324d, 0x21, 0x0, 0x0)
        /home/agallo/go/src/github.com/cloudflare/gortr/cmd/gortr/gortr.go:403 +0xdba
main.(*state).routineUpdate(0xc0000c0000, 0x7ffc1078324d, 0x21, 0x258, 0x7ffc10783296, 0x24)
        /home/agallo/go/src/github.com/cloudflare/gortr/cmd/gortr/gortr.go:507 +0x2c0
main.main()
        /home/agallo/go/src/github.com/cloudflare/gortr/cmd/gortr/gortr.go:861 +0x666

example entry used to cause this:

           {
             "asn": 65039,
             "prefix": "198.51.100.0/355",
             "comment": "I trust the TESTNET-2"
           },

If an invalid ASN is provided, there is no panic, this message is logged:

ERRO[0002] Slurm: json: cannot unmarshal number 4294967296 into Go struct field SlurmPrefixAssertion.LocallyAddedAssertions.PrefixAssertions.ASN of type uint32

And goRTR ignores all entries in the Prefix Assertion section

code used to generate this:

           {
             "asn": 4294967296,
             "prefix": "198.51.100.0/24",
             "comment": "I trust the TESTNET-2"
           },

For the local assertions, things got a bit worse- An invalid address specification, such as

          {
             "prefix": "198.51.100.0/322",
             "comment": "local something"
           },

results in goRTR filtering everything:

INFO[1204] Slurm filtering: 0 kept, 151101 removed, 2 asserted
INFO[1204] New update (2 uniques, 2 total prefixes). 0 bytes. Updating sha256 hash 06580ab7a3b1872323ef1f513acdd402e901de75f6c13304101bbb37b9c66408 -> b56168b0fac51993de909cfaf6c1c294a76513bd64649fd45e6a9b4ee2624f00
INFO[1206] Updated added, new serial 1

All entries from the upstream validator (I'm using octoRPKI in this test) are filtered. I confirmed that only 2 prefixes made it to the router I was testing with.

Maybe provide some sanity checking for the input via SLURM?

Thank you.

agallo avatar Jun 01 '20 14:06 agallo

Will fix. Thank you for reporting

lspgn avatar Jun 01 '20 18:06 lspgn

I prepared a PR, let me know if you are able to test it. It should silently discard invalid prefixes. Adding logging would be doable in the future. I prefer this instead of dropping the whole SLURM (I may be wrong). The first error is due to JSON decoding so it just does not accept the file.

lspgn avatar Jun 05 '20 23:06 lspgn

I'd be happy to test. Just let me know

agallo avatar Jun 09 '20 12:06 agallo

I merged the change on master: could you git pull and go build inside cmd/gortr?

lspgn avatar Jun 09 '20 17:06 lspgn

I don't see any more panics. Thanks for the quick fix!

agallo avatar Jun 12 '20 12:06 agallo