Libtorrent: Use of SetFileValidData (on Windows) is a security hazard

[MOZGIII]

This is just a discussion regarding the security implications of the SetFileValidData API call on Windows.

I expected libtorrent to just zero-fill a non-sparse file, but apparently users have previously reported this as problematic in terms of performance.

On the other hand, the way files are allocated currently is a security hazard: to the very least, I'd expect the file to be initially opened with exclusive access, and quickly filled with real data (if net speed can fully saturate dick write speed) or with zeroes to prevent unrestricted access to the bytes previously stored in that space that we've just allocated for our new file on the device.

This can lead to very unintended results (things like accessing removed priviliged data), and it can be specifically exploited by the malware.

So, let's discuss this. I've briefly looked through the issues and PRs and didn't find a proper discussion on this topic, sorry to bother everyone with this if the proper discussion on the matter is out there I just missed it.

[FranciscoPombal]

This has been discussed many times both here and on the qBittorrent issue tracker. Here are some relevant results (you can find all of them by searching both repositories for SetFileValidData):

https://github.com/qbittorrent/qBittorrent/issues/8756#issuecomment-382368599
https://github.com/qbittorrent/qBittorrent/issues/8309#issuecomment-359980746 and further comments in that thread.

Basically, the SetFileValidData API is only used when the user runs the program with admin rights. That being the case, I think it's fair to say that the user should know what they are doing and all bets are off.

[arvidn]

Basically, the SetFileValidData API is only used when the user runs the program with admin rights. That being the case, I think it's fair to say that the user should know what they are doing and all bets are off.

I'm not sure it's a safe assumption people running programs as admins know what they're doing. But perhaps they get what they deserve :)

It's not the default anymore (as in the XP days), so I would just think the whole feature of SetFileValidData() is quite irrelevant. Virtually nobody will have access to it (since they don't run the client as administrator).

Probably the only people who can take advantage of this have read in a forum somewhere that you can get this improvement by running as administrator.

[FranciscoPombal]

@arvidn

Now I'm curious. Is there actually a proof of concept showing this being exploited with libtorrent?

[arvidn]

no, not that I know of. As far as I can tell, the risk is that some other, malicious, non-privileged, process can read the files a libtorrent process is writing to, to possibly pick up leaked sensitive data.

What kind of data? Well, anything that has ever been stored on the disk basically.

[FranciscoPombal]

@arvidn

no, not that I know of. As far as I can tell, the risk is that some other, malicious, non-privileged, process can read the files a libtorrent process is writing to, to possibly pick up leaked sensitive data.

Interesting. I wonder how common it is for other programs out in the wild to make use of SetFileValidData. I would imagine it is less of an issue if many others do it as well. Not because the fact the others also do it makes it right, but because it means such a potential data leak of previously deleted files has implicitly become an "acceptable risk" when using Windows. I would also imagine it is not so easy and clear cut to pull off - any reads are always racing against libtorrent's writes of new data.

[arvidn]

I think it's extremely rare to use this call. Microsoft has made it Administrator-only and discouraged it for a very long time. It's presumably only still around in applications that were built around this feature back in the Windows XP times, or earlier. But It wouldn't expect consumer software use it.

Steam used to use it back when it first came out, to allocate space for its cache. Users noticed and freaked out (thinking that the steam client was spying on them, because it had "stored" deleted data in its cache file).

[FranciscoPombal]

@arvidn

Well, thanks for the insights. I must say have no real stake in this, as I seldom use Windows, and when I do, the choice between libtorrent exposing access to deleted data or performing worse when preallocation is used doesn't affect me anyway. So I have no further suggestions/recommendations. I suppose it's up to you and other concerned Windows users whether this is changed or not.

But if I had to choose, I would vote to keep it as it is. Like I said above, I think anyone running as admin should be comfortable with the risks. I think I should add a note about this in a wiki page for qBittorrent, though.

[MOZGIII]

Thanks for the insight!

In my view, security concerns are still underrated. It's a fact that the library doesn't perform adequate mitigations for the potential security issue, and, to sum up, the consensus is that it's just not a big deal. I don't have any concrete examples to demonstrate the implications, and we definitely need some to advance here, but I don't have the time to work on this now.

I was considering using the libtorrent in one of the projects I'm building, and it looks like having a dedicated non-privileged process driving its operation would be a solution here - and that's what I was planning to do anyway.

From the library user perspective though, it would make sense to have a knob to turn this off even when running with admin rights. Then, at least, this decision can be shifted from being universal and at the library level to each library adopter. And maybe even exposed as a configuration to the end-user.

[arvidn]

I agree that it would be reasonable to have an option to disable the use of SetFileValidData(). But I think the actual utility might be a bit questionable. Are there really any justification to run a libtorrent process as Administrator? (I'm a bit ignorant of windows systems, so it's a genuine question). Are all system services run as Administrator?

Unrelated to SetFileValidData(); as you point out, running libtorrent in a demoted subprocess makes a lot of sense. I spent some time a few years ago looking into ways of doing this (not in the library itself, but in a client application). I think it's unfortunate that it's as complicated as it is, for such a useful feature.

[arvidn]

In fact, I think it might make sense to disable the use of SetFileValidData() by default, and have an option to enable it. I can't imagine very many people actually benefit from this.

Alternatively, instead of running all of libtorrent as administrator, just to get efficient disk space allocation, there could be a separate tool that just allocates files, that's run as administrator.

[arvidn]

does this look reasonable? https://github.com/arvidn/libtorrent/pull/5129