Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

specconv: do not permit null bytes in mount fields #3287

Merged
merged 1 commit into from
Nov 19, 2021
Merged

specconv: do not permit null bytes in mount fields #3287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libcontainer/container_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2195,6 +2195,9 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na
var mounts []byte
for _, m := range c.config.Mounts {
if m.IsBind() {
if strings.IndexByte(m.Source, 0) >= 0 {
return nil, fmt.Errorf("mount source string contains null byte: %q", m.Source)
}
mounts = append(mounts, []byte(m.Source)...)
}
mounts = append(mounts, byte(0))
Expand Down
12 changes: 12 additions & 0 deletions libcontainer/specconv/spec_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,18 @@ func createLibcontainerMount(cwd string, m specs.Mount) (*configs.Mount, error)
mnt.Source = filepath.Join(cwd, m.Source)
}
}

// None of the mount arguments can contain a null byte. Normally such
// strings would either cause some other failure or would just be truncated
// when we hit the null byte, but because we serialise these strings as
// netlink messages (which don't have special null-byte handling) we need
// to block this as early as possible.
if strings.IndexByte(mnt.Source, 0) >= 0 ||
strings.IndexByte(mnt.Destination, 0) >= 0 ||
strings.IndexByte(mnt.Device, 0) >= 0 {
return nil, errors.New("mount field contains null byte")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be useful to include which field (for users to debug what's wrong)?

In we think it's useful, we could do a loop over the fields, and check each one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said; it would still return the first error (unless we always check all of them), so usefulness may be limited

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a security measure, and less a mechanism to tell the user to correct something. If there's a null byte in there, something is very wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the main reason for doing this in specconv is to make sure we always reject such configurations, rather than only rejecting them when we hit the actually attack-prevention check in bootstrapData.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was mostly a nit; I don't think "security measure" and "informative/detailed" error is mutually exclusive, but this situation should be a corner-case, so not really worth spending too much time on to make it "nice".

}

return mnt, nil
}

Expand Down