[ADDONS|SECURITY] Ensure our zip hasn't been tampered with before rollback

[t-nelson]

Currently it's possible to craft the file name of a malicious addon such that if the user were to roll back the legitimate addon, they would install the malicious one instead. Not a difficult scenario to imagine with a small amount of social engineering.

Keep checksums of all downloaded packages in the addons db and verify the zips before presenting them to the user for rollback.

The DB stuff needs review by someone who knows what they're doing. @jmarshallnz?

I'm also unsure whether we'd want to trust what the user has downloaded currently. AFAIK, no one is exploiting this ATM. I only happened across it trying to plug a hole for one of Pivos' customers.

[jmarshallnz]

AFAICT, there's no requirement for a repository to supply MD5 sums for add-ons. In this case you'd be trusting whatever was downloaded (same way we trust zip installs).

Perhaps I misunderstand, but afaict the hash check is done regardless of whether it's a rollback or new install anyway?

[t-nelson]

I didn't realize the MD5s weren't a requirement of the repo, perhaps we should consider an "Allow addons from untrusted sources" setting like android has. Either way, unless someone has gone out of their way to install a third party repo, they're going to be getting the addons from us and there will be sums. Starting the line of trust at the point of install for anything else is really the best we can do without putting far too tight a clamp on things, IMO.

[jmarshallnz]

What happens if the user manages to get a .zip file in packages that's corrupt? The MD5 will be in the database at that point I think? Perhaps we should only insert into the database once we've actually installed successfully?

[t-nelson]

What happens if the user manages to get a .zip file in packages that's corrupt? The MD5 will be in the database at that point I think? Perhaps we should only insert into the database once we've actually installed successfully?

If I read the code correctly, the install failing at this point shouldn't be the fault of the zip. We've already verified its integrity based on hash and a basic file listing test. Would listing have succeeded if the file was corrupt?

I think I was trying to break the chances of a download loop in the case where install failed due to file system permissions or capacity.

[Montellese]

How do you know whether the MD5 checksum from the server isn't corrupted either? And what stops someone being able to replace an old addon ZIP with a forged one from just accessing the SQLite database file and change the MD5 in there for the package?

EDIT: I don't think this is a bad idea, but it's very difficult to get it "right" especially in open source where you can't really protect your (SQLite) database etc. with username and password.

[t-nelson]

How do you know whether the MD5 checksum from the server isn't corrupted either?

Obviously, we have to trust our repo. If we can't, then we have way bigger problems. The user needs to trust any third-party repos they use. All we can do is provide legitimate third party sources some semblance of security.

And what stops someone being able to replace an old addon ZIP with a forged one from just accessing the SQLite database file and change the MD5 in there for the package?

Absolutely nothing. We'd have to enforce a passphrase to open the db and encrypt the hashes to protect against that. I doubt many users would be willing enter a passphrase to use xbmc just for security. Though that would likely be an excellent future option.

EDIT: I don't think this is a bad idea, but it's very difficult to get it "right" especially in open source where you can't really protect your (SQLite) database etc. with username and password.

Indeed this is more an, "at least try," solution than a, "bullet-proof," one. The problem isn't in being open source though, it's the necessity of a unique secret and the intrusiveness its implications to the user. Hiding your algorithm isn't, "right," when it comes to security.

[t-nelson]

Right, actually tested this time...

DB queries cleaned up and download logic reworked. I'll squash the FU before merge.

[jmarshallnz]

I wonder if we should only bother calling AddPackage() if we actually have a hash (either an existing one in the database in case of a rollback, or in m_hash)?

In all other cases we're installing from an unknown source, the concern being that you need only install a "bad" zip at some point and the hash for it is stored, and the same "good" zip will then not install as it doesn't match the hash (Note: currently doesn't happen as the database hash isn't checked in the case m_hash is empty)

i.e. if we have a hash in the database, it came from a repository (whether trusted or not). If we don't, it was from an unknown source (zip/repo without hashes etc.)

[t-nelson]

That would effectively disable the ability to rollback unknown addons,
which I'm not opposed to.

[jmarshallnz]

That's one way to do it, yes. Alternatively, no hash in the database might be interpreted as "we can't trust what we have, but couldn't trust what we had when we first installed it anyway, so go ahead"

[t-nelson]

Ok, as discussed yesterday on IRC. We now only store md5's of "trusted" sources. This only leaves the question of what to do with existing packages during the addon update. Currently I hash and store, but I think this should be dropped. @jmarshallnz?

[jmarshallnz]

Agreed, drop.

[t-nelson]

Done and squashed. If you don't have any further concerns, go ahead and
hit the button. I'll be out for festivities the rest of the evening.

[jmarshallnz]

Looks good other than the minor cosmetic stuff.

[t-nelson]

Cleaned up the minors.

jenkins build this please

[yol]

@t-nelson Sorry for digging this up after such a long time... I'm currently implementing some changes to the add-on downloads, and this PR came up tracking the origins of some code lines I'm unsure about.
Even reading the PR here, I'm not sure what the exact scenario is that you are trying to protect against. For my understanding, can you give me a concrete example how someone would craft a malicious filename and what the preconditions for that turning into a security problem are?
It's hard to not break the precautions that were taken here when you don't fully understand them :-)