Skip to content
This repository has been archived by the owner on Feb 29, 2024. It is now read-only.

Panic with invalid SLURM file #67

Open
agallo opened this issue Jun 1, 2020 · 5 comments
Open

Panic with invalid SLURM file #67

agallo opened this issue Jun 1, 2020 · 5 comments
Assignees

Comments

@agallo
Copy link

agallo commented Jun 1, 2020

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 lspgn self-assigned this Jun 1, 2020
@lspgn
Copy link
Contributor

lspgn commented Jun 1, 2020

Will fix. Thank you for reporting

lspgn added a commit that referenced this issue Jun 5, 2020
@lspgn
Copy link
Contributor

lspgn commented Jun 5, 2020

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 added a commit that referenced this issue Jun 8, 2020
@agallo
Copy link
Author

agallo commented Jun 9, 2020

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

@lspgn
Copy link
Contributor

lspgn commented Jun 9, 2020

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

@agallo
Copy link
Author

agallo commented Jun 12, 2020

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants