Possible security issue with slugs and filename

[JustinDrake]

As I understand, the slug is effectively a user-defined filename. The idea of user-defined filenames should raise security alarm bells.

Let's look at an example in listings.go.

	listingPath := path.Join(n.RepoPath, "root", "listings", slug+".json")

	// Read existing file
	file, err := ioutil.ReadFile(listingPath)

Because slug can be a string like ../../path/to/internal/file.private, we're allowing an attacker to ask the OS to read arbitrary files. This could leak the existence of files (e.g. by looking at errors, or doing a timing attack), could allow a DoS attack (e.g. by forcing openbazaar-go to read very large files), and could maybe even lead an attacker to read the contents of any file.

Similar stuff is happening with image filenames, for example in images.go:

os.Create(path.Join(imgPath, "tiny", filename))

os.Create will overwrite any existing file.

I believe I have previously flagged up the possible security concerns of user-set filenames with @hoffmabc and @cpacia on Slack. This was months ago when discussing the initial design of the protobufs. Are we really going down this route? If so, there will be little room for sloppiness, e.g. in validating slugs and filenames.

[jjeffryes]

We've gone back and forth several times on user-defined slugs. If @tyler-smith and @cpacia agree there's a vulnerability here, that's a serious issue we need to discuss.

[JustinDrake]

There are also issues in jsonapi.go, specifically POSTListing and PUTListing which both do os.Create on a user-defined slug. It suffices for one single XSS vulnerability in openbazaar-desktop for an attacker to be able to wipeout the target's computer.

[hoffmabc]

This is for the local user to create files though right? How could this be exploited to create new files or overwrite remotely?

[JustinDrake]

If by "This" you mean POSTListing and PUTListing, then this would be exploited through an XSS vulnerability on the client side.

[cpacia]

I'm not sure I see how this could be exploited. It seems like one would need malware on their machine and even there I don't see how that would impact any other users on the network.

[JustinDrake]

No malware is assumed otherwise all bets are off. I see two attack vectors for an attacker:

1. Make use of the public API to shove a user-defined filename. For example, the attacker could request to download the file /../../../familyPics/pic.png. Or maybe the attacker could get the victim to click a crafted link such as ob://@victim/../../../familyPics/pic.png.

2. In a first stage, the attaker could exploit an XSS on the client side to give him access to the internal APIs, and then in a second stage abuse those. This goes against defence in depth. For example, DeleteListing will gleefully delete any file on demand:

toDelete := path.Join(n.RepoPath, "root", "listings", slug+".json")
err := os.Remove(toDelete)

[hoffmabc]

So I think there are several things that leave Electron's security model to be desired. Not sandboxing things is the first dramatic issue, but would be difficult for us to limit due to the nature of this product (needing OS integration). I think for now the prudent thing to do would be to look at all the points in the code where we're integrating with the file system for these types of exploits and ensure we're properly filtering out paths, etc. This is app development 101, but everyone overlooks things from time to time. We need more eyes on this.

1. This one I think should be a pretty simple fix to make sure we're not requesting files outside of the scope of our application folder.

2. This is a pretty straight foward case of not filtering user input properly. There's no reason to accept blind input for file manipulation activities.

[JustinDrake]

Ok, I'm not the only one with these concerns. Very interesting discussion on this IPFS issue:

ipfs/kubo#1710

[hoffmabc]

I looked at this briefly, but it looked like they already disallow slashes in names. Doesn't that pretty much prevent the issue you're proposing?

[JustinDrake]

Just disallowing slashes in names almost certainly doesn't prevent an attacker from messing around. (There are huge amounts of complexity involved; just read the thread in a little more detail.) I can only confidently say that a conservative check such as [A-Za-z0-9_\.\-] would be OK. The spectrum between that and "everything but no slashes" is huge.

[hoffmabc]

I was just making one point, not saying that was the only protection is provides currently. I don't think it's worth our time to do something too dramatic, beyond a simple check for now considering they're working on it and it is more of an IPFS issue than an OpenBazaar one.

If you'd like to submit a PR for doing the check you suggest that would be appreciated. Beyond that you'll have to be more specific what you would like us to do to appease your concern.

[JustinDrake]

If people think that restricting slugs and filenames to [A-Za-z0-9_\.\-] is good, I'd be happy to make a pull request for that. An obvious issue with it is internationalisation (stuff like Chinese characters would be excluded).

it is more of an IPFS issue than an OpenBazaar one

I disagree. Proper validation of slugs, filenames and ob:// URIs are part of the attack surface and OpenBazaar's responsibility.

[hoffmabc]

Ok we are talking about two separate issues here. You brought up IPFS. In IPFS there's no way it will serve up those improper files. The slug validation issue is a whole other problem and valid. I would think just removing anything except for the last component would be good enough to avoid the issue you mentioned.