Invalidenum workaround #859

peterhillman · 2020-11-08T22:57:51Z

Address https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=26641
exrenvmap checks for unknown values of EnvMap attribute and avoids setting an EnvMap enum to an unknown value is undefined behavior. exrheader also has simliarly workarounds for EnvMap attributes. exrcheck also confirms that any EnvMap and DeepImageState attributes are set to valid states

Signed-off-by: Peter Hillman <[email protected]>

cary-ilm · 2020-11-20T01:05:23Z

src/bin/exrenvmap/readInputImage.cpp

+    else if (hasEnvmap (in.header()))
+    {
+        // validate type is known before using
+        const Envmap* typeInFile = &envmap (in.header());


It might be easier to read the intention here if written as:
const Envmap& typeInFile = envmap (in.header()
const int envMapAsInt = * reinterpret_cast<const int*>(&typeInFile);
since envmap() returns const&. The use of pointers to integers is confusing, and at least this limits that to a single reinterpret_cast line.

The underlying issue is this -

IMF_STD_ATTRIBUTE_IMP (envmap, Envmap, Envmap)

expands to

const TypedAttribute<type> & \ IMF_NAME_ATTRIBUTE(name) (const Header &header) \ { \ return header.typedAttribute <TypedAttribute <type> > \ (IMF_STRING (name)); \ }

which would be a TypedAttribute, where Envmap is

enum Envmap { ENVMAP_LATLONG = 0, // Latitude-longitude environment map ENVMAP_CUBE = 1, // Cube map NUM_ENVMAPTYPES // Number of different environment map types };

TypedAttribute has this:

const T & value () const;

so shouldn't the following work? It eliminates all the casting, and lets the compiler do the work in a bullet proof way.

if (ENVMAP_LATLONG == envmap (in.header()).value();

I can't see the originating bug report, but this avoids setting a value, so I think it would avoid the issue as described in the PR comments?

meshula

This is an interesting one. I have many questions :)

meshula · 2020-12-02T22:07:57Z

src/bin/exrenvmap/readInputImage.cpp

+    else if (hasEnvmap (in.header()))
+    {
+        // validate type is known before using
+        const Envmap* typeInFile = &envmap (in.header());


The underlying issue is this -

IMF_STD_ATTRIBUTE_IMP (envmap, Envmap, Envmap)

expands to

const TypedAttribute<type> & \ IMF_NAME_ATTRIBUTE(name) (const Header &header) \ { \ return header.typedAttribute <TypedAttribute <type> > \ (IMF_STRING (name)); \ }

which would be a TypedAttribute, where Envmap is

enum Envmap { ENVMAP_LATLONG = 0, // Latitude-longitude environment map ENVMAP_CUBE = 1, // Cube map NUM_ENVMAPTYPES // Number of different environment map types };

TypedAttribute has this:

const T & value () const;

so shouldn't the following work? It eliminates all the casting, and lets the compiler do the work in a bullet proof way.

if (ENVMAP_LATLONG == envmap (in.header()).value();

meshula · 2020-12-02T22:11:03Z

src/bin/exrenvmap/readInputImage.cpp

+    else if (hasEnvmap (in.header()))
+    {
+        // validate type is known before using
+        const Envmap* typeInFile = &envmap (in.header());


I can't see the originating bug report, but this avoids setting a value, so I think it would avoid the issue as described in the PR comments?

meshula · 2020-12-02T22:12:36Z

src/bin/exrheader/main.cpp

 {
-    switch (e)
+    int envmapAsInt = *( reinterpret_cast<const int*>(&e) );


Is the hard core definition of default that default means "any known legal value" and is UB for unknown values? If that's the case, should we instead be declaring Envmap as an int?

i.e.

enum Envmap : int { ...

Would declaring it as such resolve the problem without all the casting?

Changing the definition of Envmap does seem to resolve the problem. Is doing that an ABI change? I was trying to avoid that but maybe the simplicity of the code makes it worth doing.

Would it be possible to do an nm on the .so with and without the : int? If you grep the output for Envmap, we should be able to see if the mangled names with Envmap have changed or not.

The mangled names do not change, but they move around slightly, which is a little concerning.

Will all architectures guarantee that the Envmap enum was of type int before this change? I'd understood that some compilers might use an 8 bit type if everything could fit in and no type was specified. If that's the case, it might cause issues in user code even if the library itself was identical

That's inherently a problem with any sort of reinterpret_cast based solution, but I think if it was an issue it would have raised its head by now.

If the names do not change, but move around, that's not going to cause linking problems. I'm in favor of moving to an explicit type to avoid compilers having opinions about types :)

src/lib/OpenEXRUtil/ImfCheckFile.cpp

cary-ilm · 2020-12-03T01:08:14Z

This is an issue with all attributes of type enum, since values read from files need to be preserved, you can't discard invalid values. If we're going to change Envmap to an int, for consistency we should change the others. But enums exist for a reason, it would be a shame to forego the construct just because the sanitizer complains about it. The sanitizer complaint seems bogus, there's no way that can amount to a legitimate vulnerability, right? Would it be better to keep the code as is and disable the sanitizer warnings in the fuzz test?

…

On Wed, Dec 2, 2020 at 4:40 PM Nick Porcino ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/bin/exrheader/main.cpp <#859 (comment)> : > { - switch (e) + int envmapAsInt = *( reinterpret_cast<const int*>(&e) ); Would it be possible to do an nm on the .so with and without the : int? If you grep the output for Envmap, we should be able to see if the mangled names with Envmap have changed or not. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#859 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFC3DGKXYK2CWKFH6L5LENTSS3M7DANCNFSM4TOTY3TQ> .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

peterhillman · 2020-12-03T01:58:23Z

I believe compilers might optimize switch statements assuming enum variables only have valid values, so you might end up with an arbitrary case statement executing when the value is not valid. Code might also not be expecting no branch to be taken, which could cause issues.

I think the only enums of concern are Envmap and DeepImageState. All the other enums are validated when reading, and again by the sanityCheck function. That's because Compression and the enums in TileDescription are used to compute the number of chunks within the file, so if they aren't a currently known value then the file layout cannot be determined.

PixelType and LineOrder are also sanityChecked when a file is opened, so unknown values cannot be preserved (the *InputFile constructors throw an exception). That's not strictly necessary. Pixels cannot be read from an image which has an unknown PixelType but exrstdattr could theoretically still copy the data from input to output, and might be possible to read other parts of a multipart file.

Despite the potential ABI change, I like that @meshula's solution also bypasses any sanitizer warnings in user code and keeps things cleaner.

If more enums are added in the future, it might pay to have a more abstract class to handle them rather than just a plain enum, so unknown values can be handled properly and detected cleanly

cary-ilm · 2020-12-03T02:20:58Z

Could we get around this by using a union between an enum and int?

…

On Wed, Dec 2, 2020 at 5:58 PM peterhillman ***@***.***> wrote: I believe compilers might optimize switch statements assuming enum variables only have valid values, so you might end up with an arbitrary case statement executing when the value is not valid. Code might also not be expecting *no* branch to be taken, which could cause issues. I think the only enums of concern are Envmap and DeepImageState. All the other enums are validated when reading, and again by the sanityCheck function. That's because Compression and the enums in TileDescription are used to compute the number of chunks within the file, so if they aren't a currently known value then the file layout cannot be determined. PixelType and LineOrder are also sanityChecked when a file is opened, so unknown values cannot be preserved (the *InputFile constructors throw an exception). That's not strictly necessary. Pixels cannot be read from an image which has an unknown PixelType but exrstdattr could theoretically still copy the data from input to output, and might be possible to read other parts of a multipart file. Despite the potential ABI change, I like that @meshula <https://github.com/meshula>'s solution also bypasses any sanitizer warnings in user code and keeps things cleaner. If more enums are added in the future, it might pay to have a more abstract class to handle them rather than just a plain enum, so unknown values can be handled properly and detected cleanly — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#859 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFC3DGLL6OH2KIF7GWP65ZLSS3WEXANCNFSM4TOTY3TQ> .

-- Cary Phillips | R&D Supervisor | ILM | San Francisco

Signed-off-by: Peter Hillman <[email protected]>

meshula · 2020-12-03T20:38:26Z

Thinking about Peter's earlier note that he's heard of compilers picking an arbitrary size for an enum (and I've heard that as well) If it's possible that an enum can be other than 32 bits, then a union of int and enum wouldn't allow strict comparison, due to endianness. This shows the basic problem with reinterpret casting as well.

key:

hi = high byte, mh = middle hi, ml = middle lo, lo = low byte
en = 8 bits encoding an enumeration
uu = undefined memory, could be anything, zero if we're lucky

memory arrangement:

[hi][mh][ml][lo]
[en][uu][uu][uu]

vs

[lo][ml][mh][hi]
[en][uu][uu][uu]

cary-ilm · 2021-01-15T00:06:13Z

Revisiting this, have we settled on @meshula's "enum Envmap : int" solution? @peterhillman, would you like to rework the PR around that?

Signed-off-by: Peter Hillman <[email protected]>

peterhillman · 2021-01-18T01:31:20Z

This now uses "enum : int" for attributes which may be extended with future states. The sanitizer warning suppressions for Envmap and DeepImageState attributes are no longer needed so those have been removed too

meshula

lgtm :)

peterhillman added 2 commits November 5, 2020 09:32

special-case invalid EnvMap and DeepImageState enums

d6c9b91

Signed-off-by: Peter Hillman <[email protected]>

Merge branch 'master' into invalidenum_workaround

3b64e3a

cary-ilm reviewed Dec 2, 2020

View reviewed changes

Merge branch 'master' into invalidenum_workaround

07c989a

meshula reviewed Dec 2, 2020

View reviewed changes

specify 'int' type for enums which may carry unknown values

59c3c95

Signed-off-by: Peter Hillman <[email protected]>

peterhillman added 2 commits January 18, 2021 12:55

Merge branch 'master' into invalidenum_workaround

838c223

remove extraneous invalid-enum suppressions

5c5d351

Signed-off-by: Peter Hillman <[email protected]>

meshula approved these changes Jan 18, 2021

View reviewed changes

peterhillman merged commit 01d8d74 into AcademySoftwareFoundation:master Jan 25, 2021

peterhillman deleted the invalidenum_workaround branch January 25, 2021 19:38

cary-ilm added the v3.0.0-beta label Mar 21, 2021

cary-ilm added the oss-fuzz label May 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalidenum workaround #859

Invalidenum workaround #859

peterhillman commented Nov 8, 2020

cary-ilm Nov 20, 2020

meshula Dec 2, 2020

meshula Dec 2, 2020

meshula left a comment

meshula Dec 2, 2020

meshula Dec 2, 2020

meshula Dec 2, 2020

peterhillman Dec 2, 2020

meshula Dec 3, 2020

peterhillman Dec 3, 2020

meshula Dec 3, 2020

cary-ilm commented Dec 3, 2020 via email

peterhillman commented Dec 3, 2020

cary-ilm commented Dec 3, 2020 via email

meshula commented Dec 3, 2020 •

edited

Loading

cary-ilm commented Jan 15, 2021

peterhillman commented Jan 18, 2021

meshula left a comment

Invalidenum workaround #859

Invalidenum workaround #859

Conversation

peterhillman commented Nov 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meshula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cary-ilm commented Dec 3, 2020 via email

peterhillman commented Dec 3, 2020

cary-ilm commented Dec 3, 2020 via email

meshula commented Dec 3, 2020 • edited Loading

cary-ilm commented Jan 15, 2021

peterhillman commented Jan 18, 2021

meshula left a comment

Choose a reason for hiding this comment

meshula commented Dec 3, 2020 •

edited

Loading