IBX-5236: Added version parameter validation in DownloadController

[glye]

Question	Answer
JIRA issue	IBX-5236
see also	IBX-3110
Type	bug
Target eZ Platform version	v2.5
BC breaks	no

This was fixed before in v3.3: ezsystems/ezplatform-kernel#319
but could use a fix in v2.5 too. Note v2.5 is EOM, but not EOL. This is not a security issue, so should normally not be released for v2.5, but could we do it anyway for best practice reasons? TBD.

Checklist:

• Provided PR description.
• Tested the solution manually.
• Provided automated test coverage.
• Checked that target branch is set correctly (master for features, the oldest supported for bugs).
• Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
• Asked for a review (ping @ezsystems/engineering-team).

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[alongosz]

@glye we're backporting before EOL security issues only. 500 error on a malicious request is not a security issue.

[gggeek]

@glye we're backporting before EOL security issues only. 500 error on a malicious request is not a security issue.

Agree, but are you 100% sure that it is not possible to devise a string which will allow the user to load a different content / version than the intended one? That is the real issue at stake here - bypassing security policies.
And, btw, the same problem also affects content creation forms, where, in my opinion, we can not even exclude, without proper testing, that a malicious http payload could be used to poison the cache, ie. create contents/versions which will later be served to other users.

[adamwojs]

Suggested change

	$version = (int) $request->query->get('version');
	if ($version <= 0) {
	throw new NotFoundException('File', $filename);
	}
	$content = $this->contentService->loadContent($contentId, null, $version);
	$version = $request->query->getInt('version');
	if ($version <= 0) {
	throw new NotFoundException('File', $filename);
	}
	$content = $this->contentService->loadContent($contentId, null, $version);

[adamwojs]

or potentially `\Symfony\Component\HttpFoundation\Exception\BadRequestException`

[glye]

Thanks. Clearing the question of whether it potentially can be accepted for merge, before applying this.

[glye]

@alongosz This is indeed not security, I could have stated that more clearly. The question is rather: Is this an absolute rule, or can there be some wiggle room for good security practice, even if there is no known, likely or expected vulnerability here?
(The downside is of course the cost of QA and release for an EOM branch.)

[alongosz]

@alongosz This is indeed not security, I could have stated that more clearly. The question is rather: Is this an absolute rule, or can there be some wiggle room for good security practice, even if there is no known, likely or expected vulnerability here? (The downside is of course the cost of QA and release for an EOM branch.)

-1 from my side because of the cost. This repository is outside of gitflow. It would require unplanned QA & release. I don't see any value in adding this to so old version of the product, given it just results in 500 exception. The solution for that would be: "upgrade please". But it's really not up to me, but up to @adamwojs and/or @webhdx.

[alongosz]

Side note: CI is failing

[glye]

Fair points, absolutely. The CI issue is unrelated, but I might want to look into it at some point, especially if it happens in 3.3+ also.

[gggeek]

... even if there is no known, likely or expected vulnerability here? ...

See my comment above: did anyone at Ibexa actually try to create an attack where a specially crafted version number string can be used to trick the system into creating a cache-key which is valid and could correspond to a different content, or not? If not, I do not think that we can say that a vulnerability is unlikely and unexpected. Please bear in mind that there are atm many items stored in the Presistence Cache, which have cache keys built from different patterns, and those patterns do have a varying number of elements...

On a side note: thinking more about this issue, and the fact that this is not the only controller which suffers from this lack of sanitization, I came to the conclusion that the correct place to fix it is rather (or in addition) the place where the cache-key is generated, out of a pattern. The code which generates it should execute a typecast on all cache-key parts which are known to be integers. This is the most similar equivalent of doing a bind-by-param in a database query where the version nr. is an integer column.

[glye]

@gggeek We can't prove a negative. I mean, no one can prove 100% that any large piece of software is free from vulnerabilities. Two Ibexa devs have looked carefully at the call stack now and not found any vulnerability. This was also reviewed back in July 2022 when the fix was first made. Security concerns were reviewed back then as well. That cannot rule out vulnerabilities, nothing can.

In my view the sanitation should happen as soon as possible in the stack, but doing it at multiple levels for redundancy is fine, within reason. Personally I think it's better to throw exceptions than typecast, but I went with a cast + exception here to stay in sync with the original fix.

[gggeek]

@glye I won't insist: if you say that two developers looked already at this issue and were satisfied with the current situation / this fix, I have no reason not to trust them.

I will however abuse a little more your time and patience (and this ticket) to explain my line of thinking.

I am not asking Ibexa for a formal proof of safety/security here - I agree that it is both impractical in real-life and impossible in theory (halting problem and all that).

Instead, assuming I was "the Product Boss", this is what I would try to make sure is happening:

1. for some developer, preferably with a solid background in web app security, to try not just to reproduce the issue with the example payload which was given in the relevant ticket, but also to "think like a hacker" and try to find out if there are any "attack" strings theoretically possible which can achieve what I described above, namely make the system act on a different content/version/language than what is expected.
2. for some developer to review all the existing controllers for similar cases, ie. where a non-integer value can be provided where the system expects an integer but blindly pastes it into a tokenized cache key
3. for some technical lead to make a decision about whether this issue is best fixed at the controller layer, at the layer generating the cache keys, both, or elsewhere
4. for some unit tests to be added to the existing test suite(s), which cover this kind of attack, and check for the correct results. Possibly trying both a sql injection and a cache-key-corruption

Part, or even all of the above, might in fact have happened behind closed doors. But I am not privy to that. On the other hand, what I see in the public communication makes me feel that the issue might be underestimated, simply because the payload originally reported did not achieve the false cache-sharing.

[glye]

@gggeek Fair points here as well. I'll answer in the CS issue.

@adamwojs Will you make the decision on whether this is mergeable or not? I think all the pros and cons have been laid out.