[espeak-ng:master] reported: Temporary fix for issue #945 #github


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #948 Temporary fix for issue #945
By rhdunn:

Aborting is the wrong thing to do here, as it would cause applications like NVDA, Orca, or screen-dispatcher to stop working unexpectedly. That would a) make it hard to know what is going on, b) annoy users of those applications, and c) possibly prevent those users from using the device. Thus, this is just as worse as the crash.


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #948 Temporary fix for issue #945
By jbowler:

You don't have a choice; the fix is definitely temporary; it doesn't recover from the overwrite and, indeed, without adding I think 2 entries to the end of ph_list2 it does not actually prevent it. Once espeak-ng is in this state it can be exploited, a cracker can potentially take over the user's machine. The only recourse is to immediately exit the program.


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #948 Temporary fix for issue #945
By jbowler:

You don't have a choice; the fix is definitely temporary; it doesn't recover from the overwrite and, indeed, without adding I think 2 entries to the end of ph_list2 it does not actually prevent it. Once espeak-ng is in this state it can be exploited, a cracker can potentially take over the user's machine. The only recourse is to immediately exit the program. I.e. the problem is not that espeak-ng fails, it is that espeak-ng, responding to externally supplied data, allows the user's machine to be infected by malware.

#include then use assert(!"ph buffer overflow"), 0 (or , NULL to avoid the gcc warnings...)

The problem with the assert is that it calls, as a minimum, write(2) and, in fact, I think it may call fprintf(3); I think it is safe to do that in this case with modern GCC because the strings, while global, are read-only (so cannot be maliciously overwritten).


espeak-ng@groups.io Integration <espeak-ng@...>
 

[espeak-ng:master] New Comment on Pull Request #948 Temporary fix for issue #945
By jbowler:

Thinking about it, because of its nature (I'm not even going to use the two word phrase, I'm sure crackers continuously scan github for it), your downstreams (your customers) are going to prefer a patch that can be applied in isolation to existing releases.

The circular buffer solution is good for this reason but even that may be too extensive. You could try replacing the abort/assert with a return but 'return' is not valid in an expression in C, neither is "goto". Hacking every single use of n_ph_list2 to be followed by a check seems the minimal safe alternative (ph_list2 has to have an extra entry for this to be safe):

if (n_ph_list2 > N_PHONEME_LIST) return FLAG_OVERFLOW;

I guess there are only three calls to the function, checking for FLAG_OVERFLOW might offer a potential recovery. It looks like TranslateClause updates the actual parsing state so that repeated calls to TranslateWord2 just reparse the same input, but I can't tell for sure; there might a good recovery there.