Apparent (Security) Bug and Perf Improvements in `oldreadcolrs(...)`

Bug Report

I believe I have found a bug in oldreadcolrs(...).

Refer to “src/common/color.c” lines 103 through 138. The function decompresses an old-style RLE-encoded scanline. The idea is that the first four bytes indicate an RGBE color (the else block, line 131), and then a following four bytes can indicate a run using it (the if block, line 120). The new pixel is copied from the preceding pixel (line 125).

The problem is that if the else block doesn’t run first (e.g. because of a corrupt or maliciously constructed image), then the copycolr macro will attempt to copy from scanline[-1]. This will happen, for example, if the read scanline starts with bytes <1,1,1,1>. The variable rshift is initialized to 0, showing that it was considered what happens when the else block doesn’t run first, but apparently it was not done so correctly.

To fix this, I recommend initializing rshift to -1 instead, and then checking that condition inside the if block. E.g. insert after line 122:

if (rshift<0)
    return(-1); /* expected a preceding pixel to duplicate */

Missed Optimization

There is also a pretty significant missed optimization opportunity in the if block’s test. Your current use of operator & instead of && suggests performance was deemed important, so I suggest doing a single test-to-zero against a (masked) uint32, which will be much faster. E.g. for little-endian:

if ( ! (((*(uint32_t*)scanline)^0x00010101u)<<8) ) { /*...*/ }

Note that for this approach, scanline needs to be aligned 4; which probably implies COLR be aligned 4 (it should be anyway; this will result in performance improvements across the codebase). If it were me, I’d use something like:

typedef union {
	struct { uby8 r, g, b, e; };
	uby8 arr[4];
	uint32_t packed;
} COLR;

—which would also make the above optimization easier.


Developer Access

A final issue for your consideration: it doesn’t seem like you provide public repo access, nor is there a location to discreetly report security issues. You even require moderator approval even to register to post on a forum. It all seems unfriendly, and designed to discourage people to help.

Hi Agatha,

Thanks very much for your input. We could certainly make these changes to guard against corrupt or malicious input. I suppose if someone is using this code in a program running as root, it could be a problem, though that is generally not recommended.

I am less concerned about optimization in this part of the code, which is almost never used these days. Virtually all RGBE files use the newer compression scheme (if they use any at all) since multiple decades ago.

Regarding developer access, we have not made it easy, mostly because we are a small community and few people have indicated an interest. We would be very happy to have your help, especially as we start to migrate towards a more modern code structure in the years ahead.

Cheers,
-Greg

Thank you for the response and welcome!

Indeed; to be fair, I’m not sure what the threat model is. If you like, this is just a bug (assuming I’m correct, that is). I am a bit more concerned in that (various) (people) [1] have blindly ported that code, duplicating the bug in other systems of unknown scope, but it’s reassuring to hear that most images aren’t using that format. Note also that some of the major loaders (e.g. OIIO) don’t seem (?) to support it at all, and so are unaffected.


For developer access, I humbly suggest a more open policy on someplace like github (it doesn’t have to be github). Then people like me would have a lower barrier to reporting problems, or even fixing them. Public development increases the noise a bit, but also helps the community grow, helps people who rely on citable contributions to be able to contribute, and as you say is very much a part of modernization efforts.


[1] Further links omitted by security policy :V

Hi Agatha,

I would like to figure out a bug-reporting model and pathway for code suggestions that encourages your participation and others who can help.

I looked a little more closely at the code in common/color.c and realized that oldreadcolrs() is only called by freadcolrs(), which manually reads the first pixel and passes an incremented buffer, therefore should never cause a buffer underrun. So, as long as people copied this code and not some previous (possibly buggy?) version, the internet should be safe from malicious images.

Cheers,
-Greg

oldreadcolrs() is only called by freadcolrs(), which manually reads the first pixel and passes an incremented buffer, therefore should never cause a buffer underrun.

I see what you mean, however oldreadcolrs(⋯) is also called in two other places in freadcolrs(⋯), and I think both those cases can happen. This is all in (this version) of the code, I guess 5.3 official.


I would like to figure out a bug-reporting model and pathway for code suggestions that encourages your participation and others who can help.

Sounds great! Ideally for me, Radiance would be a GitHub git repo. Would you like me to elaborate on what development with this would look like more than I did above?

Hi Agatha,

I see you are correct about this – there was a potential problem with malicious/corrupt inputs. Unfortunately, the fix is not quite as simple as your suggestion, because of the way freadcolrs() loads the first few bytes and needs to allow the scanline to be back-read for a run. I did find a fix, which was to modify the routine so it always calls oldreadcolrs() with scanline+1. This should close the vulnerability.

Thanks again for bringing this to my attention.

Cheers,
-Greg

P.S. I have no (good) experience with github, though many have tried to convince me of its merits. Seems like it offers an easier onramp to developers, but it’s a heavy lift for old farts like me.

Hooray! I’m glad this got fixed, and that I could help in some small way.


P.S. As above, it needn’t be GitHub, yet such platforms are simply how most developers work these days, so it’s how you’ll attract them. I urge you to at least talk over the issue with your regular, trusted, code contributors.