cloudflare / gortr

The RPKI-to-Router server used at Cloudflare
https://rpki.cloudflare.com
BSD 3-Clause "New" or "Revised" License
309 stars 39 forks source link

Panic with invalid SLURM file #67

Open agallo opened 4 years ago

agallo commented 4 years ago

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.

lspgn commented 4 years ago

Will fix. Thank you for reporting

lspgn commented 4 years ago

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.

agallo commented 4 years ago

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

lspgn commented 4 years ago

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

agallo commented 4 years ago

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