Daniel Sternberg:

>Thanks a lot for your contribution. It need some little more polish though.

Thanks for the feedback Daniel and Gisle, I'll address the points you raised.
With Christmas upon us though, it will likely be after the celebrations when
the changes will be ready.

Meanwhile, I'll respond to the issues that require my input below:

>> full_msg = conn->data->state.uploadbuffer + 4;
>> security_features = conn->data->state.uploadbuffer + 18;
>
>I'm not a champion of the SMB code. Can we really be certain that the upload
>buffer is always having that data at that index at this point?

The SMB message (both the header and the main body) should be in the upload
buffer at this point, as smb_sign is called (if required) in smb_send_message
directly after the header is formatted and copied into the upload buffer along
with the main body of the message.

With the message in place, the index values are absolute: full_msg is being set
to the beginning of the header (so it can represent the entire message), which
will always be 4 bytes in as smb_format_header places 4 bytes of data at the
front of the header, which are necessary, but not actually a part of the header.

Then, security_features is set to the SMB header field SecurityFeatures, which
is always 14 bytes into the header (+4 for the previously mentioned 4 bytes).
Again, this is due to how smb_format_header has constructed the header
(can see the structure here:
https://msdn.microsoft.com/en-us/library/ee441774.aspx), which is always
called before smb_sign.

If this is satisfactory, I will add a comment to clarify.

>> /* calculate MAC signature */
>> /* concat the MAC key and entire SMB message */
>> MAC_data = malloc(sizeof(MAC_key) + msg_len +
>> sizeof(struct smb_header) - 4);
>
>But why the -4 ? Please add a comment to explain.

This again is the 4 bytes at the front of the header that are not part of it,
and so not wanted for the message signing, which requires the header +
main body. I'll add a comment to clarify.

>> memcpy(MAC_key, smbc->MAC_key, sizeof(MAC_key));
>
>What is the point of this copy, since you copy it to another destination again
>just 6 lines below? I don't see what the Mac_key local array is needed for?

Sorry, this was just sloppy on my part - pretty embarrassing too.
It's completely unnecessary.

>> - msg.session_key = smb_swap32(smbc->session_key);
>> + msg.session_key = smb_swap32(SMB_SESSION_KEY);
>
>Really? This makes it seem like this was totally broken before. Was it? Or how
>come you can replace a dynamic key use with a fixed one like that?

I got mistaken somewhere along the way and believed that the SessionKey
field of the SMB_COM_SESSION_SETUP_ANDX was simply ignored -
looking at the documentation again though, this is only true for Windows NT
Servers. I'll change it back.

I hope this has cleared up some of the areas of uncertainty around my code.

Merry Christmas and a Happy New Year,
Joseph Tetzlaff-Deas

===========================================================

Considering Office 365? Barracuda security and storage solutions can help. Learn more about Barracuda solutions for Office 365 at http://barracuda.com/office365.

DISCLAIMER:
This e-mail and any attachments to it contain confidential and proprietary material of Barracuda, its affiliates or agents, and is solely for the use of the intended recipient. Any review, use, disclosure, distribution or copying of this transmittal is prohibited except by or on behalf of the intended recipient. If you have received this transmittal in error, please notify the sender and destroy this e-mail and any attachments and all copies, whether electronic or printed.

-------------------------------------------------------------------
List admin: http://cool.haxx.se/list/listinfo/curl-library
Etiquette: http://curl.haxx.se/mail/etiquette.html
Received on 2015-12-21