Created attachment 18929 [details] rfc patch
(In reply to David Disseldorp from comment #2) > Created attachment 18929 [details] > rfc patch One thing I forgot to clarify in the commit message was that the overrun is limited to the start of the addrlist field within the allocated parsed_mount_info struct.
Created attachment 18930 [details] 0001-mount.cifs-fix-buffer-overrun-in-set_password.patch v2: update commit message to mention that the buffer overrun is into the adjacent struct member field.
Created attachment 18931 [details] minor cleanups atop fix
(In reply to David Disseldorp from comment #5) Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> (IIRC Steve has been taking cifs-utils PRs on https://github.com/smfrench/smb3-utils [+ optionally linux-cifs@ if patch is non-trivial/or author wants broader review])
(In reply to Enzo Matsumiya from comment #6) > (In reply to David Disseldorp from comment #5) > > Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de> Thanks Enzo! As this is a reply to comment#5 , I'll assume this tag covers the minor cleanups patches. Please also review the comment#4 bug fix: https://bugzilla.samba.org/attachment.cgi?id=18930 > (IIRC Steve has been taking cifs-utils PRs on > https://github.com/smfrench/smb3-utils [+ optionally linux-cifs@ if patch is > non-trivial/or author wants broader review]) Will do, thanks.
(In reply to David Disseldorp from comment #7) I was still in doubt about the patch for the buffer overrun, that's why I didn't put Rb. I sent my concern to linux-cifs@, but I'll paste it here: There is the overrun bug, yes, but unconditionally accounting for comma expansion here will crop the password if it has at least 1 comma and (unparsed) length == MOUNT_PASSWD_SIZE, e.g.: (password here is <511 * 'a'> + ',') # echo -e "username=administrator\npassword=$(printf 'a%.0s' {1..511})," > /tmp/creds # ./mount.cifs -o credentials=/tmp/creds //w25.vm.test/test /mnt/test Converted password too long! error 1 (Operation not permitted) opening credential file /tmp/creds Maybe password/password2 could be malloc'd with size "strlen(src) + <number of commas in src>" and bounds checked against that? Just a thought.
(In reply to Enzo Matsumiya from comment #9) > (In reply to David Disseldorp from comment #7) > > I was still in doubt about the patch for the buffer overrun, that's why I > didn't put Rb. I sent my concern to linux-cifs@ Fair enough. thanks Enzo. I've sent a response to the list, let's discuss it there.