Page MenuHomeFreeBSD

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

Authored by brooks on Jun 20 2018, 8:29 PM.

Details

Summary

The local hostname was blindly copied into the to the nn_name array.
When the hostname exceeded 16 bytes, it would overflow.

PR: 228354
Reported by: donald.buchholz@intel.com

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Jun 20 2018, 8:29 PM
ian accepted this revision.Jun 20 2018, 8:44 PM
ian added inline comments.
contrib/smbfs/lib/smb/ctx.c
572 ↗(On Diff #44185)

fwiw, these sequences can reduce to

if (strlcpy(nn.nn_name, ctx->ct_locname, sizeof(nn.nn_name)) > NM_NAMELEN)
  return NBERROR(NBERR_NAMETOOLONG);
This revision is now accepted and ready to land.Jun 20 2018, 8:44 PM

I'm not sure what the consequences of returning an error here are versus silently truncating. Before this change you'd have hostname "longerthan16chars.local" and it would get passed through as "longerthan16cha" but now mount_smbfs would fail with an error? If that assertion is true maybe it would be better just to copy and truncate at 16 than to return an error.

brooks updated this revision to Diff 44187.Jun 20 2018, 8:59 PM
  • Per discussion in the PR, truncate the host name to 15 bytes.
This revision now requires review to proceed.Jun 20 2018, 8:59 PM

I'm not sure what the consequences of returning an error here are versus silently truncating. Before this change you'd have hostname "longerthan16chars.local" and it would get passed through as "longerthan16cha" but now mount_smbfs would fail with an error? If that assertion is true maybe it would be better just to copy and truncate at 16 than to return an error.

For the local host name, it seems that truncating to 15-bytes is the right thing. For the server name, it's probably a bug if we are given a server name larger than 16 bytes and we should fail.

I'm not sure what the consequences of returning an error here are versus silently truncating. Before this change you'd have hostname "longerthan16chars.local" and it would get passed through as "longerthan16cha" but now mount_smbfs would fail with an error? If that assertion is true maybe it would be better just to copy and truncate at 16 than to return an error.

For the local host name, it seems that truncating to 15-bytes is the right thing. For the server name, it's probably a bug if we are given a server name larger than 16 bytes and we should fail.

Agreed.

jpaetzel accepted this revision.Jun 20 2018, 9:26 PM
This revision is now accepted and ready to land.Jun 20 2018, 9:26 PM
This revision was automatically updated to reflect the committed changes.