search

jborean93 / smbprotocol

Python SMBv2 and v3 Client
MIT License
318 stars 73 forks source link

Skip DFS referrals without referral_entries #149

Closed MWedl closed 2 years ago

MWedl commented 2 years ago

Hi, I have encountered an IndexError in class ReferralEntry when DFSReferralResponse contains no referral_entries (i.e. number_of_referrals is 0). ReferralEntry assumes that there is always at least one entry available.

In my pull request I decided to not create instances of ReferralEntry at all when number_of_referrals is 0. This is outside of the class and does not mitigate the root problem. However, the only place where ReferralEntry instances are created is in ClientConfig.cache_referral().

Another possibility would be to check if referral_entries are available in each method of ReferralEntry and return None. I think this would cause further problems since methods do no longer guarantee to return a value but None instead. This also needs adaptions in code calling these methods.

I would appreciate feedback what you think is the best way to solve this error.

codecov[bot] commented 2 years ago

Codecov Report

Merging #149 (072f09c) into master (88a97a0) will increase coverage by 0.15%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
+ Coverage   98.93%   99.09%   +0.15%     
==========================================
  Files          23       23              
  Lines        5052     5059       +7     
==========================================
+ Hits         4998     5013      +15     
+ Misses         54       46       -8
Flag Coverage Δ
macOS 68.19% <87.50%> (+1.01%) :arrow_up:
py3.10 99.07% <100.00%> (+0.15%) :arrow_up:
py3.6 99.07% <100.00%> (+0.15%) :arrow_up:
py3.7 99.07% <100.00%> (+0.15%) :arrow_up:
py3.8 99.06% <100.00%> (+0.15%) :arrow_up:
py3.9 99.08% <100.00%> (+0.15%) :arrow_up:
ubuntu 96.77% <100.00%> (+0.16%) :arrow_up:
windows 99.00% <100.00%> (+0.16%) :arrow_up:
x64 99.09% <100.00%> (+0.15%) :arrow_up:
x86 99.00% <100.00%> (+0.16%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
smbclient/_io.py 99.03% <100.00%> (+<0.01%) :arrow_up:
smbclient/_pool.py 95.89% <100.00%> (+4.31%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 88a97a0...072f09c. Read the comment docs.

jborean93 commented 2 years ago

Interesting, I’ll have to have a closer look at the DFS docs to see when this would occur. Do you have more information about the environment where this occurred for you?

MWedl commented 2 years ago

This error occured on .DFSFolderLink which seems to be the a remainder/side-effect of the powershell cmdlet New-DfsnFolder. I just encountered this while crawling some shares and do no have more details. I hope this helps you.

jborean93 commented 2 years ago

No worries, I'll have to try and replicate but considering it's happening in real life I don't see why this check can't occur

jborean93 commented 2 years ago

The docs seems to at least mention this is possible

If the referral request is successful, but the NumberOfReferrals field in the referral header (as specified in section 2.2.4) is 0, the DFS server could not find suitable targets to return to the client. In this case, the client MUST fail the original I/O operation with STATUS_OBJECT_PATH_NOT_FOUND.

Seems like more handling will need to be done to check that lookup_referral returns an actual value otherwise STATUS_OBJECT_PATH_NOT_FOUND should be raised. I'll have a quick play and see if I can get something together to push to this PR.

jborean93 commented 2 years ago

I've pushed a commit that adds the exception handling as well as some hacky tests. If you get a change it would be great if you could try it out on your environment to make sure I didn't break anything. I would expect a DFS share with no referrals to raise ObjectPathNotFound.

MWedl commented 2 years ago

Thanks for your commit and looking up how to handle empty referrals in the documentation.

You missed one place (smbclient._io._resolve_dfs) where ObjectPathNotFound should also be thrown. I have added the check and a test case for it.

Now everything works as expected in my environment. Feel free to merge.

jborean93 commented 2 years ago

You missed one place (smbclient._io._resolve_dfs) where ObjectPathNotFound should also be thrown. I have added the check and a test case for it.

Ah thanks for the pickup, I completely forgot about this use of DFS.

Appreciate the time and effort fixing this up.