Bug 228462

Summary: net/samba47: Samba's vfs_streams_xattr triggers corruption of first byte in AFP_AfpInfo stream/xattr
Product: Ports & Packages Reporter: slow
Component: Individual Port(s)Assignee: Timur I. Bakeyev <timur>
Status: Closed FIXED    
Severity: Affects Many People CC: timur
Priority: ---    
Version: Latest   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Possible patch for 4.7, may apply to other versions
none
Modified version of the proposed fix
none
Additional fix none

Description slow 2018-05-24 14:50:14 UTC 
The Samba FreeBSD port patch https://svnweb.freebsd.org/ports/head/net/samba47/files/patch-source3__modules__vfs_streams_xattr.c?revision=464431&view=markup changes vfs_streams_xattr to not read and write an additional trailing byte (cf the comment lines containing "// ? -1" in the patch), but when creating a stream the trailing byte is still stored (cf streams_xattr_open() the code after the comment "Darn, xattrs need at least 1 byte").

Due to a vicious interaction with a bug that is present in the latest macOS 10.13.4 (not sure about earlier versions) what happens is this:

- the client send a request to create a stream "file:AFP_AfpInfo"

- the server creates the xattr for the stream and writes a 0 byte

- the client sends a request to read 60 bytes at offset 0 from the stream

- the server returns a one byte sized buffer containing a 0 instead of returning nread=0 and status=NT_STATUS_END_OF_FILE

- the final nail in the coffin is that the client, when writing the AFP_AfpInfo blob whos first four byte start with a magic string "AFP" takes the 0 byte the server returned and overwrites the first byte of the magic string

The fix for this twofold: first, we must fix vfs_streams_xattr to not store an initial zero byte when creating an xattr. Second, we must prepare vfs_fruit to allow such broken AFP_AfpInfo blobs, otherwise users who adding vfs_fruit run into the issue that vfs_fruit has a builtin check for the magic string...

Have patch, need bug number...

Fwiw, this is a bug only present in the FreeBSD Samba port.
Comment 1 slow 2018-05-24 14:53:16 UTC 
Created attachment 193656 [details]
Possible patch for 4.7, may apply to other versions
Comment 2 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-05-24 16:00:23 UTC 
Thanks a lot, Ralph, for investigating this and for the patch.

Much appreciated!
Comment 3 slow 2018-05-24 20:27:08 UTC 
Just to make it clear, the provided patch is still WIP...
Comment 4 slow 2018-05-29 07:21:55 UTC 
Btw, in case this wasn't clear enough: the patch https://svnweb.freebsd.org/ports/head/net/samba47/files/patch-source3__modules__vfs_streams_xattr.c?revision=464431&view=markup basically corrups the streams as it returns the internal 0 byte to the client.

A client which stored an 2 xattr value of 0xbe 0xef, so two bytes, will read an xattr of 3 bytes 0xbe 0xef 0x00 with the patch.

Any client who stored xattrs with and without the patch will have a mixup of the old storage format and the new one. Not sure what the best way out of this is.
Comment 5 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-05-29 12:12:40 UTC 
(In reply to slow from comment #4)
Unfortunately, it's clear, Ralph. Next question is what to do with it, you are right.
Comment 6 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-06-06 01:48:15 UTC 
(In reply to Timur I. Bakeyev from comment #5)

Hi, Ralph!

Meanwhile I've slightly modify your proposed patch, see the attachment. Basically, there are two small differences:

1. There is no need to pass address of the `null = \0'` variable to the VFS call, while we set buffer size to 0. It's better/easier to pass NULL and that's also documented behavior for the API, so is more consistent.

2. I've rearranged checks for the AFP_Signature and AFP_Version, as logically version makes sense only if we recognized the format by the signature. Result at the end should be the same, but it's a bit cleaner.

Otherwise I think it's a nice catch and good fix for the bug in the current FreeBSD's version of vfs_streams_xattr.

Thank you!
Comment 7 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-06-06 01:49:42 UTC 
Created attachment 194034 [details]
Modified version of the proposed fix
Comment 8 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-06-19 01:12:09 UTC 
The modified patch is applied to samba48 port, as of 4.8.2.
Comment 9 slow 2018-06-26 09:00:30 UTC 
Created attachment 194644 [details]
Additional fix

Looks like this one somehow got lost. It takes care of fixing up broken AFP_AfpInfo streams on the fly when the client reads them.
Comment 10 slow 2018-06-26 09:01:03 UTC 
Reopening to pick up additional patch.
Comment 11 Timur I. Bakeyev freebsd_committer freebsd_triage 2018-06-30 22:47:32 UTC 
(In reply to slow from comment #10)

Thanks for the update, Ralph! I've added your patch to the newer ports of 4.7.8 and 4.8.3. 

Much appreciated!