Bug 228354 - mount_smbfs - long hostname causes stack overflow
Summary: mount_smbfs - long hostname causes stack overflow
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Some People
Assignee: Brooks Davis
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2018-05-19 00:49 UTC by Don Buchholz
Modified: 2018-06-28 21:25 UTC (History)
6 users (show)

See Also:
brooks: mfc-stable11+
brooks: mfc-stable10+


Attachments
check hostname length (792 bytes, patch)
2018-06-20 18:25 UTC, Brooks Davis
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Buchholz 2018-05-19 00:49:27 UTC
Attempting to mount a CIFS share from a Windows server using the command

  # mount_smbfs -W WKGRP //myid@winsrv/share_name /mnt
  Password: 

fails with the message

  ... mount_smbfs[793]: stack overflow detected; terminated

and command exit status = 134. 

It does not matter if the password supplied is correct or bogus.
Arch is 'amd64'.  This command succeeds when using FreeBSD 11.1.
Comment 1 Don Buchholz 2018-05-19 17:59:54 UTC
The 12-CURRENT pre-release was installed from "-disc1" images from
https://download.freebsd.org/ftp/snapshots/amd64/amd64/ISO-IMAGES/12.0/

Both 20180426 and 20180514 releases were used and found to have this problem.
Comment 2 Eric Joyner freebsd_committer 2018-06-04 17:29:58 UTC
Is there anyone that can look at this?

Don -- there's a 5/29 snapshot available; would you try that to see if anything's changed?
Comment 3 Don Buchholz 2018-06-05 00:57:52 UTC
Eric -- still broken in May-29 snapshot.
- Don
Comment 4 Don Buchholz 2018-06-19 01:00:03 UTC
The "FreeBSD-12.0-CURRENT-amd64-20180611-r334983-disc1.iso" image still dumps core when I try and use mount_smbfs(8).  The command succeeds in a FreeBSD-11.1 system.
Comment 5 Don Buchholz 2018-06-20 01:33:51 UTC
The answer:  "27"
The question:  "What is the maximum number of characters you can have in your hostname when using mount_smbfs(8)?"

  hostname="BaseOS-FreeBSD120CURR-ia32e"   Works!
  hostname="BaseOS-FreeBSD12-CURR-ia32eX"  Breaks!

The earlier attempts had been using an algorithmically generated 32-char hostname.
Comment 6 Brooks Davis freebsd_committer 2018-06-20 18:25:40 UTC
Created attachment 194438 [details]
check hostname length
Comment 7 Brooks Davis freebsd_committer 2018-06-20 18:29:33 UTC
This problem here appears to be unchecked copies from a larger array into a 17-byte array in struct nb_name.  I've attached a compiled, but untested patch to check the size of the hostname before copying.

This restriction seems dumb and it looks like struct nb_name isn't used in kernel so it may be that we can just increase NB_NAMELEN in sys/netsmb/netbios.h.  I don't know and am not a position to test.
Comment 8 haron86 2018-06-20 20:18:44 UTC
Due to NetBIOS name limitations (the NetBIOS name of the computer is limited to 15 bytes), it is better to cut the long hostnames when copying from smb_ctx.ct_locname to nb_name.nn_name. Just replace strcpy with strncpy (nn.nb_name, ctx->ct_locname, NB_NAMELEN).
Comment 9 Brooks Davis freebsd_committer 2018-06-20 20:31:36 UTC
https://reviews.freebsd.org/D15936
Comment 10 Brooks Davis freebsd_committer 2018-06-20 20:32:50 UTC
(In reply to Aleksandr Ivanov from comment #8)
Is it 15 or 16?  sys/netsmb/netbios.h implies it's 16 plus a NUL.  If that's wrong we probably have other problems.
Comment 11 Brooks Davis freebsd_committer 2018-06-20 20:38:30 UTC
(In reply to Brooks Davis from comment #10)
I see that wikipedia says 15 + a suffix.  That probably suggests we should be doing strlcpy(..., NB_NAMELEN); and if some code wants to add the SUFFIX it must take care to  to add the terminator.
Comment 12 commit-hook freebsd_committer 2018-06-25 16:43:18 UTC
A commit references this bug:

Author: brooks
Date: Mon Jun 25 16:42:50 UTC 2018
New revision: 335641
URL: https://svnweb.freebsd.org/changeset/base/335641

Log:
  Fix a stack overflow in mount_smbfs when hostname is too long.

  The local hostname was blindly copied into the to the nn_name array.
  When the hostname exceeded 16 bytes, it would overflow.  Truncate the
  hostname to 15 bytes plus a 0 terminator which is the "workstation name"
  suffix.

  Use defensive strlcpy() when filling nn_name in all cases.

  PR:		228354
  Reported by:	donald.buchholz@intel.com
  Reviewed by:	jpaetzel,  ian (prior version)
  Discussed with:	Security Officer (gtetlow)
  MFC after:	3 days
  Security:	Stack overflow with the hostname.
  Sponsored by:	DARPA, AFRL
  Differential Revision:	https://reviews.freebsd.org/D15936

Changes:
  head/contrib/smbfs/lib/smb/ctx.c
  head/contrib/smbfs/lib/smb/nbns_rq.c
Comment 13 commit-hook freebsd_committer 2018-06-28 20:33:51 UTC
A commit references this bug:

Author: brooks
Date: Thu Jun 28 20:33:12 UTC 2018
New revision: 335774
URL: https://svnweb.freebsd.org/changeset/base/335774

Log:
  MFC r335641:

  Fix a stack overflow in mount_smbfs when hostname is too long.

  The local hostname was blindly copied into the to the nn_name array.
  When the hostname exceeded 16 bytes, it would overflow.  Truncate the
  hostname to 15 bytes plus a 0 terminator which is the "workstation name"
  suffix.

  Use defensive strlcpy() when filling nn_name in all cases.

  PR:		228354
  Reported by:	donald.buchholz@intel.com
  Reviewed by:	jpaetzel,  ian (prior version)
  Discussed with:	Security Officer (gtetlow)
  Security:	Stack overflow with the hostname.
  Sponsored by:	DARPA, AFRL
  Differential Revision:	https://reviews.freebsd.org/D15936

Changes:
_U  stable/11/
  stable/11/contrib/smbfs/lib/smb/ctx.c
  stable/11/contrib/smbfs/lib/smb/nbns_rq.c
Comment 14 commit-hook freebsd_committer 2018-06-28 21:23:46 UTC
A commit references this bug:

Author: brooks
Date: Thu Jun 28 21:23:05 UTC 2018
New revision: 335781
URL: https://svnweb.freebsd.org/changeset/base/335781

Log:
  MFC r335641:

  Fix a stack overflow in mount_smbfs when hostname is too long.

  The local hostname was blindly copied into the to the nn_name array.
  When the hostname exceeded 16 bytes, it would overflow.  Truncate the
  hostname to 15 bytes plus a 0 terminator which is the "workstation name"
  suffix.

  Use defensive strlcpy() when filling nn_name in all cases.

  PR:		228354
  Reported by:	donald.buchholz@intel.com
  Reviewed by:	jpaetzel,  ian (prior version)
  Discussed with:	Security Officer (gtetlow)
  Security:	Stack overflow with the hostname.
  Sponsored by:	DARPA, AFRL
  Differential Revision:	https://reviews.freebsd.org/D15936

Changes:
_U  stable/10/
  stable/10/contrib/smbfs/lib/smb/ctx.c
  stable/10/contrib/smbfs/lib/smb/nbns_rq.c