netdev - Re: [PATCH RFC net] tipic: guard against buffer overrun in bearer_name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <22710fbc-cbd9-4eb0-8e0f-7c2fd1f6d43a@kili.mountain>
Date: Thu, 11 May 2023 16:03:42 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Simon Horman <horms@...nel.org>
Cc: Jon Maloy <jmaloy@...hat.com>, Ying Xue <ying.xue@...driver.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Per Liden <per.liden@...pam.ericsson.com>, netdev@...r.kernel.org,
	tipc-discussion@...ts.sourceforge.net, smatch@...r.kernel.org
Subject: Re: [PATCH RFC net] tipic: guard against buffer overrun in
 bearer_name_validate()

On Wed, May 10, 2023 at 02:48:11PM +0200, Simon Horman wrote:
> Smatch reports that copying media_name and if_name to name_parts may
> overwrite the destination.
> 
>  .../bearer.c:166 bearer_name_validate() error: strcpy() 'media_name' too large for 'name_parts->media_name' (32 vs 16)
>  .../bearer.c:167 bearer_name_validate() error: strcpy() 'if_name' too large for 'name_parts->if_name' (1010102 vs 16)
> 
> This does seem to be the case, although perhaps it never occurs in
> practice due to well formed input.
> 
> Guard against this possibility by using strscpy() and failing if
> truncation occurs.
> 

The existing code is safe as you say.  I still feel like strscpy() is
better than strcpy() unless it's a fast path.

The Smatch strlen() code is not very good.

   137  static int bearer_name_validate(const char *name,
   138                                  struct tipc_bearer_names *name_parts)
   139  {
   140          char name_copy[TIPC_MAX_BEARER_NAME];
   141          char *media_name;
   142          char *if_name;
   143          u32 media_len;
   144          u32 if_len;
   145  
   146          /* copy bearer name & ensure length is OK */
   147          if (strscpy(name_copy, name, TIPC_MAX_BEARER_NAME) < 0)
   148                  return 0;

Smatch tracks strlen() but it tracks it as a maximum.  So here smatch
says that strlen name_copy is 31.
TODO 1: It should really track it as a range 0-31.
TODO 2: Create module to track strlen if the strlen is stored in a
        variable.
TODO 3: Create a new module to say if a string is NUL terminated.  (not related to this bug)
TODO 4: Create a new module to say if a string is not NUL terminated. (not related)

   149  
   150          /* ensure all component parts of bearer name are present */
   151          media_name = name_copy;
   152          if_name = strchr(media_name, ':');

TODO 5: Add strchr() handling to Smatch *DONE in my tree*

   153          if (if_name == NULL)
   154                  return 0;
   155          *(if_name++) = 0;

TODO: 6: Before Smatch saw ++ and set strlen to unknown.  It should be
         set to 29.  *DONE in my tree*

TODO 7:  Create a new module to say that if_name points to the middle of
         the media_name buffer.  The *if_name = 0; means that media_name
         strlen is now 29.

   156          media_len = if_name - media_name;

TODO 8: This is saying that "media_len = strlen(media_name) + 1".
        Tricky to do.

   157          if_len = strlen(if_name) + 1;

TODO 9: The existing code has a bug here because it thinks if_name is
        length 29 so that means if_len is 30.  I hacked around this in
        my tree so now it says it is 1-30 but TODO 1 is the correct
        fix.

   158  
   159          /* validate component parts of bearer name */
   160          if ((media_len <= 1) || (media_len > TIPC_MAX_MEDIA_NAME) ||
   161              (if_len <= 1) || (if_len > TIPC_MAX_IF_NAME))
   162                  return 0;

I think TODO 2 would make this work automatically for if_name.

   163  
   164          /* return bearer name components, if necessary */
   165          if (name_parts) {
   166                  strcpy(name_parts->media_name, media_name);
   167                  strcpy(name_parts->if_name, if_name);
   168          }
   169          return 1;
   170  }

It's a lot of work to handle this correctly.  Most of it is not
complicated, except TODO 7-8 which are very hard.

regards,
dan carpenter