mainGnuTLS - Support: sr #107527, Use of dangerous/banned functions...

 
 

sr #107527: Use of dangerous/banned functions (Analysis)

Submitter:  Jeffrey Walton <noloader>
Submitted:  Fri 19 Nov 2010 03:03:42 AM UTC
   
 
Category:  None Priority:  5 - Normal
Severity:  3 - Normal Status:  Need Info
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Operating System:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 19 Nov 2010 08:50:11 AM UTC, comment #2: 


> This SR will analyze the code around a cluster of calls to
> strcpy in serv.c. The lines of interest in peer_print_info
> (from the SR $107525 audit) are:
>
> 451: size_t len = 5 * 1024 + strlen (header);
> 457: http_buffer = malloc (len);
> ...
>
> 461: strcpy (http_buffer, HTTP_BEGIN);
> 462: strcpy (&http_buffer[sizeof (HTTP_BEGIN) - 1], DEFAULT_DATA);
> 463: strcpy (&http_buffer[sizeof (HTTP_BEGIN) + sizeof (DEFAULT_DATA) - 2], HTTP_END);
> 465: *ret_length = sizeof (DEFAULT_DATA) + sizeof (HTTP_BEGIN) + sizeof (HTTP_END) - 3;


This code block copies 3 hard-coded strings with total length of ~330 characters to buffer that has at least 5*1024 bytes.

> From the Open Group Base Specifications for strlen(), it
> appears no constraints are placed on the string 's' and no
> errors are defined for the function, which implies all
> arguments - including NULL - are acceptable. Yet a test
> program faults on GNU Linux with a NULL argument:


Right, so you can crash your test program by doing strlen(NULL).  You can check get_response() to see if peer_print_info() can ever be called with NULL header.

Tomas Hoger <thoger>
Fri 19 Nov 2010 06:35:07 AM UTC, comment #1: 

You make several points in this bug report, and it is difficult to followup on each one -- could you enumerate each problem you have identified?  Some of it is discussion, and that is more useful on the list.

Also your line numbers are way off, you should use master unless you have found a clear security issue that must be fixed in the stable 2.10.x branch.  We don't apply any non-critical fixes on the stable branch.

/Simon

Simon Josefsson <jas>
Group administrator
Fri 19 Nov 2010 03:03:42 AM UTC, original submission:  

SR #107522 flagged the general use of unsafe string handling functions. SR #107525 detailed particular instances of the use of unsafe string handling functions; and highlighted GnuTLS's inability to detect failure.

This SR will analyze the code around a cluster of calls to strcpy in serv.c. The lines of interest in peer_print_info (from the SR $107525 audit) are:

451: size_t len = 5 * 1024 + strlen (header);
457: http_buffer = malloc (len);
...

461: strcpy (http_buffer, HTTP_BEGIN);
462: strcpy (&http_buffer[sizeof (HTTP_BEGIN) - 1], DEFAULT_DATA);
463: strcpy (&http_buffer[sizeof (HTTP_BEGIN) + sizeof (DEFAULT_DATA) - 2], HTTP_END);
465: *ret_length = sizeof (DEFAULT_DATA) + sizeof (HTTP_BEGIN) + sizeof (HTTP_END) - 3;

=====
header, ehich is subsequently used in a call to strlen, is not checked for validity. In addition, there does not appear to be documentation for peer_print_info, so its unclear if Null/Empty values are acceptable to the function.

From the Open Group Base Specifications for strlen(), it appears no constraints are placed on the string 's' and no errors are defined for the function, which implies all arguments - including NULL - are acceptable. Yet a test program faults on GNU Linux with a NULL argument:

Program received signal SIGSEGV, Segmentation fault.
__strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
31 ../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
in ../sysdeps/x86_64/multiarch/../strlen.S
(gdb) bt full
#0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:31
No locals.
#1  0x000000000040066b in peer_print_info_kindof (header=0x0,
    ret_length=0x7fffffffe2ac) at replace-strcpy.c:43
        http_buffer = 0x400755 "H\205\355t\034\061\333\017\037@"
        len = 140737488348072
#2  0x000000000040062b in main (argc=1, argv=0x7fffffffe398)
    at replace-strcpy.c:14
        written = 0
        response = 0x7fffffffe390 "\001"

(Sorry for not checking ISO C - I don't have a licensed copy of the standard).

=====
The header in line 451 is passed in to peer_print_info. Based on the header length, a buffer is allocated. The 5 * 1024 appears to be a heuristic.

=====
At line 457, memory is obtained for a HTTP response. The call to malloc is checked for failure.

=====
The strcpy's at lines 461, 462, and 463 assume success since the function cannot convey failures. The assumption is then propagated to the caller via the dereference/assignment at line 465.

=====
The dereference at line 465 assumes the pointer is valid.

=====
If the invocation of peer_print_info is unsuccessful, ret_length could be in an indeterminate state from the caller's perspective.

=====
An improved strategy might include changing calls to strcpy with calls to snprintf since snprintf offers deterministic failure:

  if(ret_length)
    *ret_length = -1;

  size_t len = 5 * 1024 + strlen (header);
  ...

  size_t len2 = 0; /* scratch */
  int count = 0, written = 0;

  len2 = strlen(DEFAULT_DATA);
  if(!(len - written >= len2)) { /*handle failure*/ }

  count = snprintf(&http_buffer[written], len-written, "%s", DEFAULT_DATA);
  if(count < 0) { /*handle failure*/ }
  written += count;
  ...

  if(ret_length)
    *ret_length = written;

Jeffrey Walton <noloader>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nmav (Updated the item)
  • -email is unavailable- added by thoger (Posted a comment)
  • -email is unavailable- added by jas (Posted a comment)
  • -email is unavailable- added by noloader (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-04-10 nmav StatusNone Need Info
        Open/ClosedOpen Closed
    2010-11-19 noloader Attached File- Added peer_print_info_kindof.c, #22057

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code