Bug 15369 - smbc_open no longer works with flags O_WRONLY | O_CREAT | O_APPEND
Summary: smbc_open no longer works with flags O_WRONLY | O_CREAT | O_APPEND
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: libsmbclient (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-05 00:55 UTC by Troy Patteson
Modified: 2023-05-05 18:06 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Troy Patteson 2023-05-05 00:55:17 UTC
It is common practice to open a log file with O_WRONLY | O_CREAT | O_APPEND when appending log entries. This no longer works usng libsmbclient as of samba v4.15.10. Specifically this functionality was broken with this commit:

| * 77d1d989d1c - smbd: implement access checks for SMB2-GETINFO as per MS-SMB2 3.3.5.20.1 - Ralph Boehme


diff --git a/source3/smbd/smb2_getinfo.c b/source3/smbd/smb2_getinfo.c
index 9ff97c4d80d..184ca0f900b 100644
--- a/source3/smbd/smb2_getinfo.c
+++ b/source3/smbd/smb2_getinfo.c
@@ -303,6 +303,34 @@ static struct tevent_req *smbd_smb2_getinfo_send(TALLOC_CTX *mem_ctx,
 
                ZERO_STRUCT(write_time_ts);
 
+               /*
+                * MS-SMB2 3.3.5.20.1 "Handling SMB2_0_INFO_FILE"
+                *
+                * FileBasicInformation, FileAllInformation,
+                * FileNetworkOpenInformation, FileAttributeTagInformation
+                * require FILE_READ_ATTRIBUTES.
+                *
+                * FileFullEaInformation requires FILE_READ_EA.
+                */
+               switch (in_file_info_class) {
+               case FSCC_FILE_BASIC_INFORMATION:
+               case FSCC_FILE_ALL_INFORMATION:
+               case FSCC_FILE_NETWORK_OPEN_INFORMATION:
+               case FSCC_FILE_ATTRIBUTE_TAG_INFORMATION:
+                       if (!(fsp->access_mask & SEC_FILE_READ_ATTRIBUTE)) {
+                               tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
+                               return tevent_req_post(req, ev);
+                       }
+                       break;
+
+               case FSCC_FILE_FULL_EA_INFORMATION:
+                       if (!(fsp->access_mask & SEC_FILE_READ_EA)) {
+                               tevent_req_nterror(req, NT_STATUS_ACCESS_DENIED);
+                               return tevent_req_post(req, ev);
+                       }
+                       break;
+               }
+
                switch (in_file_info_class) {
                case FSCC_FILE_FULL_EA_INFORMATION:
                        file_info_level = SMB2_FILE_FULL_EA_INFORMATION;



Opening a file with smbc_open() results in libsmb_file.c:SMBC_open_ctx() being called. When calling with O_WRONLY | O_CREAT | O_APPEND  flags, the call to cli_open() succeeds and the file is created if it didn't already exist.

However, the O_APPEND flag causes this code to run:

                /*
                 * If the file was opened in O_APPEND mode, all write
                 * operations should be appended to the file.  To do that,
                 * though, using this protocol, would require a getattrE()
                 * call for each and every write, to determine where the end
                 * of the file is. (There does not appear to be an append flag
                 * in the protocol.)  Rather than add all of that overhead of
                 * retrieving the current end-of-file offset prior to each
                 * write operation, we'll assume that most append operations
                 * will continuously write, so we'll just set the offset to
                 * the end of the file now and hope that's adequate.
                 *
                 * Note to self: If this proves inadequate, and O_APPEND
                 * should, in some cases, be forced for each write, add a
                 * field in the context options structure, for
                 * "strict_append_mode" which would select between the current
                 * behavior (if FALSE) or issuing a getattrE() prior to each
                 * write and forcing the write to the end of the file (if
                 * TRUE).  Adding that capability will likely require adding
                 * an "append" flag into the _SMBCFILE structure to track
                 * whether a file was opened in O_APPEND mode.  -- djl
                 */
                if (flags & O_APPEND) {
                        if (SMBC_lseek_ctx(context, file, 0, SEEK_END) < 0) {
                                (void) SMBC_close_ctx(context, file);
                                errno = ENXIO;
				TALLOC_FREE(frame);
                                return NULL;
                        }
                }


which results in the unhelpful ENXIO error being returned when it appears the error is probably EPERM because the file was opened write-only and does not have read permissions required by the cli_qfileinfo_basic() called by SMBC_lseek_ctx().


Testing methodology:

Modify the testtruncate example program (examples/libsmbclient/testtruncate.c) to append instead by changing the first smbc_open() call as follows:

    //if ((fd = smbc_open(argv[1], O_WRONLY | O_CREAT | O_TRUNC, 0)) < 0)
    if ((fd = smbc_open(argv[1], O_WRONLY | O_CREAT | O_APPEND, 0)) < 0)


Add a return before the second smbc_open / ftruncate:

    printf("Original size: %lu\n", (unsigned long) st.st_size);
   return 0;
    if ((fd = smbc_open(argv[1], O_WRONLY, 0)) < 0)


A working result (for all commits before commit 77d1d989d1c):

troyp@neo:~/git/samba$ ./bin/testtruncate smb://127.0.0.1/test/wibble
Workgroup: [WORKGROUP] 
Username: [troyp] 
Password: 
Original size: 29

troyp@neo:~/git/samba$ ./bin/testtruncate smb://127.0.0.1/test/wibble
Workgroup: [WORKGROUP] 
Username: [troyp] 
Password: 
Original size: 58


A failed result (for all commits from commit 77d1d989d1c):

troyp@neo:~/git/samba$ ./bin/testtruncate smb://127.0.0.1/test/wibble
Workgroup: [WORKGROUP] 
Username: [troyp] 
Password: 
smbc_open: No such device or address



The workaround is to open the file read/write rather than read-only:

    if ((fd = smbc_open(argv[1], O_RDWR | O_CREAT | O_APPEND, 0)) < 0)
Comment 1 Ralph Böhme 2023-05-05 18:06:29 UTC
Hm, I guess my commit just unmaskes another bug... Iirc smbd_calculate_maximum_allowed_access_fsp() should ensure the handle has FILE_READ_ATTRIBUTES as if the client can access a path by nature it has FILE_READ_ATTRIBUTES. So there's possibly something not correctly pushing this access right into fsp->access_mask.