Project

General

Profile

Bug #34134

Fix corruption of first byte in AFP_AfpInfo stream/xattr in Samba

Added by Andrew Walker 11 months ago. Updated 4 months ago.

Status:
Done
Priority:
No priority
Assignee:
John Hixson
Category:
OS
Target version:
Seen in:
Severity:
High
Reason for Closing:
Reason for Blocked:
Other: make a note in comments
Needs QA:
No
Needs Doc:
No
Needs Merging:
No
Needs Automation:
No
Support Suite Ticket:
ZCX-891-77675
Hardware Configuration:
ChangeLog Required:
No

Description

See also FreeBSD bug ticket here: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=228462

This has been observed on my own systems as well. Struct for AFP_AfpInfo is as follows:

typedef struct {
/*Ofs*/
/*  0*/    le32 signature;        /* Must be "AFP\0". */
/*  4*/    le32 version;        /* Must be 0x00010000. */
/*  8*/    le32 fileid;        /* This is the inode number as returned by the
                   AFP connection.  For some reason this is not
                   the same as the underlying NTFS inode
                   number.  Need to set this to zero on create,
                   preserve it on write, and ignore it on
                   read. */
/* 12*/    le32 backup_time;    /* Backup time for the file/dir in AppleDouble
                   time format (see above). */
/* 16*/    FINDER_INFO finder_info;/* Finder Info (32 bytes, see above). */
/* 48*/    PRODOS_INFO prodos_info;/* ProDOS Info (6 bytes, see above). */
/* 54*/ u8 reserved[6];        /* Reserved. */
/* sizeof() = 60 (0x3c) bytes */

AFP_AfpInfo prior to streams_xattr change is as follows:

root@cat_herder:/mnt/dozer/SOFIA_PHOTOS # getextattr -qq user 'DosStream.AFP_AfpInfo:$DATA' Finder\ Refresh.app/ | hexdump -C
00000000  41 46 50 00 00 00 01 00  00 00 00 00 80 00 00 00  |AFP.............|
00000010  50 4c 41 50 6c 74 61 70  04 10 00 00 00 00 00 00  |PLAPltap........|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000030

After new streams_xattr:

00000000  00 46 50 00 00 00 01 00  00 00 00 00 00 00 00 80  |.FP.............|
00000010  50 4c 41 50 6c 74 61 70  04 10 00 00 00 00 00 00  |PLAPltap........|
00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000030

Ralph's explanation is as follows:
"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..."

Malformed signatures may cause undefined client behavior. We may be seeing this in the wild. Placing severity "high".

old_streams_xattr.pcap (1.21 MB) old_streams_xattr.pcap Upstream streams_xattr Andrew Walker, 06/27/2018 10:32 AM
new_streams_xattr.pcap (1.47 MB) new_streams_xattr.pcap FreeBSD streams_xattr Andrew Walker, 06/27/2018 10:32 AM
Finder Refresh.zip (74.3 KB) Finder Refresh.zip Test file for OSX client Andrew Walker, 06/27/2018 12:32 PM

Related issues

Related to FreeNAS - Bug #30702: Alternate Data Streams (ADS) with Fruit on SMB share problemClosed
Copied to FreeNAS - Bug #36183: Fix corruption of first byte in AFP_AfpInfo stream/xattr in SambaDone

Associated revisions

Revision e764a9dd
Added by Timur Bakeyev 10 months ago

Merge pull request #56 from freenas/FIX-34134-master

Fix 34134 master

Ticket: #34134

Revision edb6998e (diff)
Added by John Hixson 10 months ago

Restore original vfs_streams_xattr.c

Ticket: #34134

Revision ab7b2d99 (diff)
Added by John Hixson 10 months ago

Configurable buffer sizes for extended attributes

- This is a way simpler approach and is also based on what the Samba
guys plan to do. This patch is based off of a patch by Ralph Boehme.
It's known to work and keeps things simple. It also helps with keeping
our tree in sync with Samba's tree.

- Minimum of 256 bytes
- Maximum of 16M
- Optimized for the 256 byte case, if larger, 16M is used

Ticket: #34134

Revision 24e97569 (diff)
Added by John Hixson 10 months ago

Standalone tool to fix AFP files with EA corruption

Ticket: #34134

Revision d9e4acff (diff)
Added by John Hixson 10 months ago

Bump samba port revision

Ticket: #34134

Revision 386bef03 (diff)
Added by John Hixson 10 months ago

Add ability to write out null byte at end of EA's

Ticket: #34134

Revision e3f98c0b (diff)
Added by John Hixson 10 months ago

Add ability to write out null byte at end of EA's (#1468)

  • Add ability to write out null byte at end of EA's

Ticket: #34134

  • Free stuff
  • cosmetic stuff

Revision 024485c0 (diff)
Added by John Hixson 10 months ago

Reworked with recursive option, still a WIP

Ticket: #34134

Revision a5940b47 (diff)
Added by John Hixson 10 months ago

Standalone tool to fix AFP files with EA corruption

Ticket: #34134

Revision a88aa1ba (diff)
Added by John Hixson 10 months ago

Restore original vfs_streams_xattr.c

Ticket: #34134
(cherry picked from commit edb6998ee61a87f2e77b4b61b219d119afe0819f)

Revision 302a6eb1 (diff)
Added by John Hixson 10 months ago

Configurable buffer sizes for extended attributes

- This is a way simpler approach and is also based on what the Samba
guys plan to do. This patch is based off of a patch by Ralph Boehme.
It's known to work and keeps things simple. It also helps with keeping
our tree in sync with Samba's tree.

- Minimum of 256 bytes
- Maximum of 16M
- Optimized for the 256 byte case, if larger, 16M is used

Ticket: #34134
(cherry picked from commit ab7b2d99aa6e03efcaefd04f79835efdc7b73398)

Revision f8349a2c (diff)
Added by John Hixson 10 months ago

Bump Samba port revision

Ticket: #34134

Revision df0e3fd5 (diff)
Added by John Hixson 9 months ago

Configurable buffer sizes for extended attributes

- This is a way simpler approach and is also based on what the Samba
guys plan to do. This patch is based off of a patch by Ralph Boehme.
It's known to work and keeps things simple. It also helps with keeping
our tree in sync with Samba's tree.

- Minimum of 256 bytes
- Maximum of 16M
- Optimized for the 256 byte case, if larger, 16M is used

Ticket: #34134
(cherry picked from commit ab7b2d99aa6e03efcaefd04f79835efdc7b73398)

Revision b751f267 (diff)
Added by John Hixson 9 months ago

Standalone tool to fix AFP files with EA corruption

Ticket: #34134
(cherry picked from commit 24e97569c7f54f727e0b8d2b2710412079e7cd18)

Revision 3ed8061b (diff)
Added by John Hixson 9 months ago

Add ability to write out null byte at end of EA's (#1468)

  • Add ability to write out null byte at end of EA's

Ticket: #34134

  • Free stuff
  • cosmetic stuff

(cherry picked from commit e3f98c0b678b1df027c35ad262929c090af3428f)

Revision db11b26d (diff)
Added by John Hixson 9 months ago

Standalone tool to fix AFP files with EA corruption

Ticket: #34134
(cherry picked from commit a5940b470952a600f1ef372046551b84b1cc17f8)

(11.1-stable)
Ticket: #34134

History

#1 Updated by Andrew Walker 11 months ago

  • Assignee changed from Release Council to Timur Bakeyev

#2 Updated by Andrew Walker 11 months ago

  • File new_streams_xattr.pcap added
  • File old_streams_xattr.pcap added

#3 Updated by Sam Fourman 11 months ago

#4 Updated by Dru Lavigne 11 months ago

  • Status changed from Unscreened to In Progress
  • Assignee changed from Timur Bakeyev to Andrew Walker
  • Target version changed from Backlog to 11.2-RC2

#5 Updated by Sam Fourman 11 months ago

  • Support Suite Ticket changed from n/a to ZCX-891-77675

#6 Updated by Andrew Walker 11 months ago

  • Assignee changed from Andrew Walker to Timur Bakeyev
  • Target version changed from 11.2-RC2 to 11.1-U5

#7 Updated by Dru Lavigne 11 months ago

  • Target version changed from 11.1-U5 to 11.2-RC2

#12 Updated by Dru Lavigne 11 months ago

  • File deleted (old_streams_xattr.pcap)

#13 Updated by Dru Lavigne 11 months ago

  • File deleted (new_streams_xattr.pcap)

#14 Updated by Dru Lavigne 11 months ago

  • Subject changed from vfs_streams_xattr triggers corruption of first byte in AFP_AfpInfo stream/xattr to Fix corruption of first byte in AFP_AfpInfo stream/xattr in Samba
  • Status changed from In Progress to Ready for Testing
  • Target version changed from 11.2-RC2 to 11.1-U6
  • Needs Doc changed from Yes to No
  • Needs Merging changed from Yes to No

#15 Updated by Dru Lavigne 11 months ago

  • Private changed from Yes to No

#16 Updated by Dru Lavigne 11 months ago

  • Status changed from Ready for Testing to In Progress
  • Assignee changed from Timur Bakeyev to Andrew Walker

Andrew: we'll also need the stable PR to get this into testing.

#17 Updated by Dru Lavigne 11 months ago

  • Assignee changed from Andrew Walker to Timur Bakeyev

As per discusion with Timur, more work needs to happen on this ticket.

#20 Avatar?id=13649&size=24x24 Updated by Ben Gadd 10 months ago

  • Status changed from In Progress to Ready for Testing

#21 Updated by Dru Lavigne 10 months ago

#22 Avatar?id=13649&size=24x24 Updated by Ben Gadd 10 months ago

  • Status changed from Ready for Testing to Blocked
  • Reason for Blocked set to Other: make a note in comments

#26 Updated by Andrew Walker 10 months ago

#30 Updated by Dru Lavigne 10 months ago

  • Copied to Bug #36183: Fix corruption of first byte in AFP_AfpInfo stream/xattr in Samba added

#31 Updated by John Hixson 10 months ago

  • Assignee changed from Timur Bakeyev to John Hixson

#32 Updated by John Hixson 10 months ago

I've proposed my solution to Timur & Mav. I'm waiting for Timur to respond. I think we need to restore vfs_streams_xattr.c to the original version that Samba uses. The re-work of the module is overly complicated and has many problems with it not even counting the corruption that is occurring. We can make large extended attributes configurable in a much easier way that is compatible with how Samba plans to do them. I've added this to the pull request. I've also written a standalone tool to fix files that were corrupted.

Samba master PR: https://github.com/freenas/samba/pull/59
FreeNAS master PR: https://github.com/freenas/freenas/pull/1455

#33 Updated by John Hixson 10 months ago

  • Status changed from Blocked to Ready for Testing

#34 Updated by John Hixson 10 months ago

  • Status changed from Ready for Testing to In Progress

#41 Updated by Timur Bakeyev 10 months ago

I think there is a mix up between #34134 and #36183. First one is for 11.1-U6 and the second one is for 11.2b1.

My understanding is that John provided his fix for 11.2b1 in #36183, while this ticket is for the 11.1-U6 and the fix for it should be

https://github.com/freenas/samba/pull/57

In general I wouldn't do any radical changes in an upgrade between U5 to U6. But correct me, if I'm wrong.

#42 Updated by John Hixson 10 months ago

Timur Bakeyev wrote:

I think there is a mix up between #34134 and #36183. First one is for 11.1-U6 and the second one is for 11.2b1.

My understanding is that John provided his fix for 11.2b1 in #36183, while this ticket is for the 11.1-U6 and the fix for it should be

https://github.com/freenas/samba/pull/57

In general I wouldn't do any radical changes in an upgrade between U5 to U6. But correct me, if I'm wrong.

There is no mix up here. The same fix for 11.2b1 will be going out in 11.1-U6.

#43 Updated by John Hixson 10 months ago

So there isn't really a problem here. Large EA's work fine. The only "problem" that happens is when a file with a large EA over what is a predefined size is copied to the local file system, it gets truncated. This isn't the general use case and this has worked this way all along. I don't even think this can be classified as a bug. I think we are good to go here and there are no more show stoppers.

#44 Updated by Dru Lavigne 10 months ago

  • Status changed from In Progress to Ready for Testing

#45 Updated by Timur Bakeyev 9 months ago

  • Related to Bug #30702: Alternate Data Streams (ADS) with Fruit on SMB share problem added

#46 Updated by Dru Lavigne 9 months ago

#47 Updated by John Hixson 9 months ago

#48 Updated by Dru Lavigne 9 months ago

  • Status changed from Ready for Testing to In Progress
  • Needs Merging changed from No to Yes

#49 Updated by Dru Lavigne 9 months ago

  • Status changed from In Progress to Ready for Testing
  • Needs Merging changed from Yes to No

#50 Updated by Bonnie Follweiler 8 months ago

  • Status changed from Ready for Testing to Passed Testing
  • Needs QA changed from Yes to No

Passed testing in FreeNAS 11.1-U6 Internal4

#51 Updated by Dru Lavigne 8 months ago

  • Status changed from Passed Testing to Done

Also available in: Atom PDF