bvh importer assertion failures when attempting to import invalid files

[JC3]

BVH importer raises a large number of the following assertion failures when it attempts to load invalid files containing binary data:

minkernel\crts\ucrt\src\appcrt\convert\isctype.cpp(36) : Assertion failed: c >= -1 && c <= 255

While I have not investigated the exact source, this particular assertion is almost certainly due to passing signed chars to one of the cctype functions, which are undefined for values outside of EOF & the range of unsigned chars.

Presuming that's the case, the fix is to convert to unsigned chars (I promise this is the correct fix; I just don't feel like citing C99 / C++ standards right now).

[krishty]

Mind having a look at ply_reader.cc, PlyReader::SplitWords() as well? Smells bad to me …

[krishty]

Okay, I just searched the codebase for toupper(), tolower(), isalpha(), isdigit() and it’s full of places where untrusted text like texture names is passed without first casting to unsigned char.

@JC3 if this is too much for you, I’ll put it on my TODO for another day.

[JC3]

It's too much for both of us, so let's both put it on our TODO lists for another day and we'll see who gets there first. 😂 I actually will have time eventually just maybe not in the next day or two, as I'm about to travel out of town for work.

…

On Tue, May 4, 2021 at 2:27 PM krishty ***@***.***> wrote: Okay, I just searched the codebase for toupper(), tolower(), isalpha(), isdigit() and it’s full of places where untrusted text like texture names is passed without first casting to unsigned char. @JC3 <https://github.com/JC3> if this is too much for you, I’ll put it on my TODO for another day. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3867 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLDV4ABMGBVHJUELQD7DUDTMA37TANCNFSM44CI5HXA> .

[JC3]

Turns out, I've got time. PR forthcoming. Notes:

• I looked for all 14 functions from cctype.
• I went with the immediate fix: Converting params to unsigned chars as needed. I noticed, though, lots of random and duplicate string-related code spattered around, so the philosophically correct fix would be to kind of unify all that and provide safer implementations in a central place. Except, I don't care, and I suspect nobody else does, either. So, I'm not doing that.
• I stayed away from zlib; I figure whatever's going on in there is already under control.
• I did, however, fix a potential issue in gtest, since it was pretty clear that there was a minor oopsie, and what the fix was. I may or may not be motivated to submit it to official gtest, too. I stuck it on official gtest, too in isalnum -> IsAlNum for correct handling of signed chars google/googletest#3393. It probably hasn't caused any issues.
• The replacements in StringUtils.h (e.g. ai_tolower, etc.) are fine as-is; they're hard-coded to C locale and the logic in them does the right thing (relatively speaking).

I'm not sure how to prevent potential issues from reappearing in the future, though. Maybe there's a code analysis tool that can spot ctype calls with char parameters that can be added to CI tests? No idea.

PR forthcoming, as soon as I figure out how tf GitHub forks work (I'm a dinosaur).

---

PS There was a chunk of gtest code that gave me the warm fuzzies (because I'm a huge nerd) as I was doing this, though:

assimp/contrib/gtest/include/gtest/internal/gtest-port.h

Lines 2256 to 2294 in 38df0f4

	// Utilities for char.
	// isspace(int ch) and friends accept an unsigned char or EOF. char
	// may be signed, depending on the compiler (or compiler flags).
	// Therefore we need to cast a char to unsigned char before calling
	// isspace(), etc.
	inline bool IsAlpha(char ch) {
	return isalpha(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsAlNum(char ch) {
	return isalnum(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsDigit(char ch) {
	return isdigit(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsLower(char ch) {
	return islower(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsSpace(char ch) {
	return isspace(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsUpper(char ch) {
	return isupper(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsXDigit(char ch) {
	return isxdigit(static_cast<unsigned char>(ch)) != 0;
	}
	inline bool IsXDigit(wchar_t ch) {
	const unsigned char low_byte = static_cast<unsigned char>(ch);
	return ch == low_byte && isxdigit(low_byte) != 0;
	}
	inline char ToLower(char ch) {
	return static_cast<char>(tolower(static_cast<unsigned char>(ch)));
	}
	inline char ToUpper(char ch) {
	return static_cast<char>(toupper(static_cast<unsigned char>(ch)));
	}

I feel so ... validated.

[krishty]

Great, thanks a lot! This may save us from a few fuzzing issues :D

I'm not sure how to prevent potential issues from reappearing in the future, though. Maybe there's a code analysis tool that can spot ctype calls with char parameters that can be added to CI tests? No idea.

I remember learning this from one tool, that’s why I was so alarmed when I read your issue. But I don’t remember whether it was clang-tidy or just GCC with all warnings.

Also noticed that Assimp compiles with /W3 instead of /W4 on Visual C++; another thing for my TODO (now that you made a little room there).

[JC3]

Closed by #3880