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

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

Details

Reviewers

jpaetzel
• ian
trasz

Commits

rS335781: MFC r335641:
rS335774: MFC r335641:
rS335641: Fix a stack overflow in mount_smbfs when hostname is too long.

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

brooks created this revision.Jun 20 2018, 8:29 PM

Harbormaster completed remote builds in B17499: Diff 44185.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.

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

Harbormaster completed remote builds in B17500: Diff 44187.Jun 20 2018, 8:59 PM

In D15936#337219, @jpaetzel wrote:

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.

In D15936#337243, @brooks wrote:

In D15936#337219, @jpaetzel wrote:

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.