[m-rev.] for review: avoid "poison null" security vulnerabilities

[Julien Fischer]

On Mon, 12 Mar 2007, Simon Taylor wrote:

>
> Estimated hours taken: 15
> Branches: main
>
> Make all functions which create strings from characters throw an exception
> or fail if the list of characters contains a null character.
>
> This removes a potential source of security vulnerabilities where one
> part of the program performs checks against the whole of a string passed
> in by an attacker (processing the string as a list of characters or using
> `unsafe_index' to look past the null character), but then passes the string
> to another part of the program or an operating system call that only sees
> up to the first null character.  Even if Mercury stored the length with
> the string, allowing the creation of strings containing nulls would be a
> bad idea because it would be too easy to pass a string to foreign code
> without checking (as in the first example link).
>
> For examples see:
> <http://insecure.org/news/P55-07.txt>
> <http://www.securiteam.com/securitynews/5WP0B1FKKQ.html>
> <http://www.securityfocus.com/archive/1/445788>
> <http://www.securityfocus.com/archive/82/368750>
> <http://secunia.com/advisories/16420/>
>
> NEWS:
> 	Document the change.
>
> library/string.m:
> 	Throw an exception if null characters are found in
> 	string.from_char_list and string.from_rev_char_list.
>
> 	Add string.from_char_list_semidet and string.from_rev_char_list_semidet
> 	which fail rather throwing an exception.  This doesn't match the
> 	normal naming convention, but string.from_{,rev_}char_list are widely
> 	used, so changing their determinism would be a bit too disruptive.

I suggest calling them string.semidet_from_char_list and 
string.semidet_from_rev_char_list which is closer to the normal naming
convention.  (I think the last time this came up we just ending breaking
backwards comptability and sticking with the "correct" naming convention
but in this case I agree, it would be too disruptive.)

> 	Don't allocate an unnecessary extra word for each string created by
> 	from_char_list and from_rev_char_list.
>
> 	Explain that to_upper and to_lower only work on un-accented
> 	Latin letters.
>
> library/lexer.m:
> 	Check for invalid characters when reading Mercury strings and
> 	quoted names.
>
> 	Improve error messages by skipping to the end of any string
> 	or quoted name containing an error.  Previously we just stopped
> 	processing at the error leaving an unmatched quote.
>
> library/io.m:
> 	Make io.read_line_as_string and io.read_file_as_string fail
> 	if the input file contains a null character.

`fail' is not a particularly good word to use in this context. 
>
> 	Fix an XXX: '\0\' is not recognised as a character constant,
> 	but char.det_from_int can be used to make a null character.
>
> library/char.m:
> 	Explain the workaround for '\0\' not being accepted as a char
> 	constant.
>
> 	Explain that to_upper and to_lower only work on un-accented
> 	Latin letters.
>
> compiler/layout.m:
> compiler/layout_out.m:
> compiler/c_util.m:
> compiler/stack_layout.m:
> compiler/llds.m:
> compiler/mlds.m:
> compiler/ll_backend.*.m:
> compiler/ml_backend.*.m:
> 	Don't pass around strings containing null characters (the string
> 	tables for the debugger).  This doesn't cause any problems now,
> 	but won't work with the accurate garbage collector.  Use lists
> 	of strings instead, and add the null characters when writing the
> 	strings out.
>
> tests/hard_coded/null_char.{m,exp}:
> 	Change an existing test case to test that creation of a string
> 	containing a null throws an exception.
>
> tests/hard_coded/null_char.exp2:
> 	Deleted because alternative output is no longer needed.
>
> tests/invalid/Mmakefile:
> tests/invalid/null_char.m:
> tests/invalid/null_char.err_exp:
> 	Test error messages for construction of strings containing null
> 	characters by the lexer.
>
> tests/invalid/unicode{1,2}.err_exp:
> 	Update the expected output after the change to the handling of
> 	invalid quoted names and strings.
>
>

...

> Index: library/char.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/char.m,v
> retrieving revision 1.56
> diff -u -u -r1.56 char.m
> --- library/char.m	13 Feb 2007 01:58:52 -0000	1.56
> +++ library/char.m	9 Mar 2007 08:36:00 -0000
> @@ -43,6 +43,11 @@
>     % represent characters using Unicode, but store files in an 8-bit national
>     % character set.
>     %
> +    % Note that '\0' is not accepted as a Mercury character constant.

Add:

 	for the null character.

to the end of that sentence.

The rest look fine.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------