Repository navigation
honor permissions in SSH_FXP_MKDIR attrs - #700
likitha011 wants to merge 4 commits into
Conversation
The Server ignored the ATTRS sent with SSH_FXP_MKDIR and always created the directory with mode 0755, so a client asking for a private directory (for example sshfs forwarding `mkdir -m 700`) got one that any local user could read. RequestServer handlers never saw the attrs either, since requestFromPacket did not copy them into the Request. Keep the attrs on sshFxpMkdirPacket, apply the requested permission bits in the Server the same way OPEN does, and pass the flags and attrs on to RequestServer handlers. Without SSH_FILEXFER_ATTR_PERMISSIONS the Server still uses 0755.
| if fs, ok := p.Attrs.(*FileStat); ok { | ||
| b = marshalFileStat(b, p.Flags, fs) | ||
| } |
There was a problem hiding this comment.
Please copy the implementation in sshFxpOpenPacket https://github.com/pkg/sftp/blob/master/packet.go#L816-L824
There was a problem hiding this comment.
Done, sshFxpMkdirPacket now has a marshalPacket that mirrors the open one ([]byte, os.FileInfo, *FileStat, else marshal), with MarshalBinary just appending header and payload.
| mode := os.FileMode(0o755) | ||
| var err error | ||
| if p.Flags&sshFileXferAttrPermissions != 0 { | ||
| var fs *FileStat | ||
| if fs, err = p.unmarshalFileStat(p.Flags); err == nil { | ||
| mode = fs.FileMode() & os.ModePerm | ||
| } | ||
| } | ||
| if err == nil { | ||
| err = os.Mkdir(s.toLocalPath(p.Path), mode) | ||
| } |
There was a problem hiding this comment.
This should probably be converted to a respond receiver method to match sshFxpOpenPacket
This will allow for an early return design, rather than needing to juggle declarations and scopes.
There was a problem hiding this comment.
Done, sshFxpMkdirPacket has a respond method now, laid out like the open one with early returns, and the inline case in handlePacket is gone so it dispatches through serverRespondablePacket. Tests, integration and race runs still pass.
| } | ||
|
|
||
| func (p *sshFxpMkdirPacket) respond(svr *Server) responsePacket { | ||
| mode := os.FileMode(0o755) |
There was a problem hiding this comment.
Huh, https://github.com/openssh/openssh-portable/blob/master/sftp-server.c#L1206-L1207
mode = (a.flags & SSH2_FILEXFER_ATTR_PERMISSIONS) ?
a.perm & 07777 : 0777;Turns out we want to be using 0o777 for directories, and 0o666 for files.
(It being up to the umask to remove undesired permission bits.)
There was a problem hiding this comment.
Good catch. Mkdir now defaults to 0o777 and open to 0o666, leaving the rest to umask.
| Flags uint32 | ||
| Path string | ||
| Attrs any |
There was a problem hiding this comment.
Oh, also while we’re here, we can reorder these to be in packet encoded order?
There was a problem hiding this comment.
Done, it is ID, Path, Flags, Attrs now.
Repro: send SSH_FXP_MKDIR with SSH_FILEXFER_ATTR_PERMISSIONS and mode 0700 (sshfs does this for
mkdir -m 700). The Server creates the directory as 0755, so other local users can list and read it.Cause:
sshFxpMkdirPacket.UnmarshalBinaryreads the attrs flags but drops the attrs body, andhandlePacketcallsos.Mkdir(path, 0o755)unconditionally.requestFromPacketalso never copies the flags/attrs for Mkdir, so RequestServer handlers cannot honor the mode either.Fix: keep the attrs on the packet and apply the permission bits the same way OPEN already does (
FileMode() & os.ModePerm). Without the permissions flag the Server still uses 0755. Covered byTestMkdirWithPermissionsandTestRequestFromMkdirPacketAttrs, which fail with the old server.go/request.go.