ronnie sahlberg wrote:
There are only very very few dissectors thus far that do this kind of
string manipulation.
Most dissectors dont do any string manipulation at all.
Are you sure? Didn't had a look, but IMO at least the higher
(application) level dissectors are having some more string processing,
most are done by hand and most of this in real danger of having security
problems.
I think that in a future audit phase one should go through all string
processing and convert as many as possible into
standard value_strings (some dissectors still use switch-statements to
pick out static strings) and append the strings to col_info and the
proto_item one component at a time instead of first building these
temporary strings.
My aim was to first get rid of all sprintf() which was done a week
ago,
Which was a very good idea :-)
and then all GString and g_string_...
and standardize on g_snprintf() for this phase.
Which might be a bad idea, the way you've done it IMHO (at least at the
place I've mentioned). You've reduced the risk of memory leaks but
raised the risk of security problems.
The GString has the main advantage of being security safe (at least if
used correct, but it's easy to do so) and the main disadvantage of
probable memory leaks.
IMHO to reduce sec problems we should implement our own (probably
simplified/modified) implementation of GString using ep_malloc() and use
it in the dissectors.
I also audited that all value_strings were terminated properly.
Again, a very good idea :-)
Next phase i would like to eliminate all strcat and strcpy.
Replacing it by strlcat and strlcpy or do things by hand or something
different?
For replacements see above.
In a future phase i would like to eliminate all character arrays that
are either global variables or stack variables to become either sp or
ep emem allocated memory.
In particular i would like to move every one of them off the stack
since this would reduce the danger of buffer overflows.
eliminating all use of arrays that live on the stack would virtually
eliminate the danger of buffer overflows as an attack vector.
Good :-)
eliminating all global/static arrays would reduce the runtime
requirement of memory for ethereal slightly.
Well, maybe even dramatically ;-)
And as a nice side effect it will reduce the number of things to solve
for having multiple capture files into one Ethereal instance.
After that i think it would be nice to go over all loops and verify
that offset is always increased in each iteration or that the loop
terminates with an exception.
Good :-)
After that i think we should be in pretty decent shape, at least
better shape than we were in in 0.10.11
Yes, this dramatically reduces the possible security (and stability)
problems we had in the past.
As a conclusion:
You're doing a great job making Ethereal even better :-)))
Regards, ULFL