OwlCyberDefense / setools

setools has moved to https://github.com/SELinuxProject/setools
Other
91 stars 34 forks source link

Fix build failure with GCC 7 due to possible truncation of snprintf output #176

Closed vorlonofportland closed 6 years ago

vorlonofportland commented 7 years ago

setools fails to build under GCC7 -Wformat -Werror with the following error:

x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -g -O2 -fdebug-prefix-map=/<>=. -fstack-protector-strong -Wformat -Werror=format-security -Wno-sign-compare -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -Ilibqpol -Ilibqpol/include -I/usr/include/python3.6m -c libqpol/policy_extend.c -o build/temp.linux-amd64-3.6/libqpol/policy_extend.o -Werror -Wextra -Waggregate-return -Wfloat-equal -Wformat -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-include-dirs -Wnested-externs -Wold-style-definition -Wpointer-arith -Wredundant-decls -Wstrict-prototypes -Wunknown-pragmas -Wwrite-strings -Wno-missing-field-initializers -Wno-unused-parameter -Wno-cast-qual -Wno-shadow -Wno-unreachable-code -fno-exceptions libqpol/policy_extend.c: In function 'policy_extend': libqpol/policy_extend.c:161:27: error: '%04zd' directive output may be truncated writing between 4 and 10 bytes into a region of size 5 [-Werror=format-truncation=] snprintf(buff, 9, "@ttr%04zd", i + 1); ^~~~~ libqpol/policy_extend.c:161:22: note: directive argument in the range [1, 4294967295] snprintf(buff, 9, "@ttr%04zd", i + 1); ^~~

Exceeding 10,000 attributes is necessarily going to result in collisions inserting into the hash table given this naming scheme, and we already error out on the first collision; but there will be holes since types are not handled the same as attributes. Short of making backwards-incompatible changes to the entry names, this is probably the best way to fix this build failure while reducing the chances of a hash collision in the unlikely event that the hashtable is (nearly) full.

I don't know how many attributes is typical, but I suspect this is a completely untested corner case in practice since each of 10000-10009 would hash as '@ttr1000' currently and cause a collision. So I don't think there's a reason to be concerned about backwards-compatible hashing of the probably-already-colliding attributes. I haven't read the code deeply enough to know whether extending the max size of the name field would be a better option instead, but that's also certainly a possibility.

Closes: https://github.com/TresysTechnology/setools/issues/174

bigon commented 7 years ago

OK I can confirm that this is fixing the FTBFS and the tests are passing fine

pebenito commented 7 years ago

The code is synthesizing a name for an attribute in really old binary policies, which didn't keep the attribute name. We can't have a collision, so buffer size should be increased.

dac4755 commented 7 years ago

Chris, do you need the same change for line 270?

@@ -270,7 +270,7 @@ static int qpol_policy_fill_attr_holes(qpol_policy_t * policy) for (i = 0; i < db->p_types.nprim; i++) { if (db->type_val_to_struct[i]) continue;

bigon commented 6 years ago

As https://github.com/TresysTechnology/setools/pull/181 is now merged, I guess that this pull request can be closed?