Requesting a private channel for a memory-safety issue in the NTRIP client

[router0mail]

Hello,

I have a memory-safety issue to report in the NTRIP client path — it is triggered by what the caster sends back during the handshake, before any RTCM data is processed, and I have it reproducing against current sources with a sanitizer trace and a crash on a stock build. The same code is present in both the demo5 and upstream trees and, going by a GitHub code search, is vendored into a little over a hundred other repositories.

I would rather not put the details in a public issue before there is a fix. Private vulnerability reporting is switched off on this repo — if you turn it on under Settings > Security > Private vulnerability reporting, I can file the whole thing there, including the reproducer and a patch I have already tested against your tree. If you would prefer email or some other route, name it and I will use that instead.

For what it is worth on prioritising: this one is reachable by whoever answers the TCP connection, and NTRIP v1 is plaintext HTTP with no server authentication, so that is not only the caster operator.

I also have a second, lower-severity out-of-bounds write in the RTCM3 SSR VTEC decoder that I will include in the same report.

Happy to wait as long as you need. Just let me know where to send it.

Stay in touch,
Alex J.

[ourairquality]

The NTRIP code has been rework substantially, include more bounds checks, see #936 Could use testing.

Might the VTEC issue have been addressed in #851

[rtklibexplorer]

I've enabled private vulnerability reporting

[router0mail]

Thanks — I checked both against current master rather than assuming.

The NTRIP rework in #936 does not cover this one. The line I am reporting is still there, unchanged, at src/stream.c:1740, and the guard immediately above it is still the same partial one.

The VTEC decoder is likewise unchanged by #851. decode_ssr8() at src/rtcm3.c:2103-2115 still has the loop bounds I am reporting, and the array dimensions in src/rtklib.h are the same. #851 added the nlay>4 check and the memset of the coefficient arrays, both good, but neither touches the degree and order.

Details are in the private advisory rather than here, since @rtklibexplorer turned reporting on for exactly that — both come with a reproducer and captured sanitizer output, and the NTRIP one with a tested patch. @rtklibexplorer, if you want @ourairquality on the advisory as a collaborator, that is yours to add and I would welcome the review.

[ourairquality]

At stream.c 1740 there is a strcpy. The rewrite replaces all the unsafe string operations in stream.c with safe versions that at least make these only a denial of services issue. The code is riddled with unsafe string operations and commonly uses fixed length string buffers. Lets see if the stream.c rewrite is ever merged, and if so then the safe string operations can be used elsewhere too.

PR #851 does add guards to two loops, the reading of the VTEC coefficients. Sorry can't spot the remaining issue there.

There is a branch with safe getbit* operations, including for rtcm3.c. This is intended to catch missing guards - a defence in depth. Not going to put in the effort to submit that again unless there is a good chance of it being merged.

[rtklibexplorer]

@ourairquality: I’ve added you as a collaborator, so you should now be able to see the issue being discussed. I’ve also created a new experimental branch that I think could potentially become a longer-term alternative to main.

I’ve given you direct push access to experimental, and I’d encourage you to use it for larger or more aggressive changes that may be harder to bring into main immediately. As you know, I’ve sometimes been cautious about accepting some of your larger changes into main—not because I don’t think they are good ideas, but mainly because I want to be conservative about stability and, to a lesser extent, growth in memory usage.

If you’d like to take this on, I’d be happy to consider you the owner/maintainer of the experimental branch and give you a lot of freedom to shape its direction. I’d also suggest updating the README on that branch to describe what you’d like it to become, what kinds of changes belong there, and how it differs from main.

At the same time, I definitely hope you’ll continue contributing changes to main. I really appreciate all the work you’ve done on RTKLIB, and I’m hoping this gives us more room to explore bigger ideas without making it harder to keep main stable.

[ourairquality]

@ourairquality: I’ve added you as a collaborator, so you should now be able to see the issue being discussed. I’ve also created a new experimental branch that I think could potentially become a longer-term alternative to main.

Thank you.

Would you be open to reformatting the code at this point, before anything is merged into the experimental branch, as I would like to do so, would like to apply a code format standard. Doing this now will make it simpler to merge and cherry pick changes between branches. There was no feedback on the desired format for the qtapp directory, even though the style is distinct, so would just apply the same format to all the code.

Could I also suggest merging #901 as it touches a lot of code and is a trivial style change.

RTKLIB has some distinct areas of code such as the stream, rinex, raw converters, and front ends. Changes to these are less likely to make subtle changes to the solutions and errors are likely more obvious, and these would be the areas of initial focus, changes that might be simpler to merge in to the main branch.