Redmine

Michael Vorobiev: Thank you for finding this and providing the patches!

I think it is clearer to explicitly deny access rather than silently ignoring an explicit parameter. As such, I prefer the first patch. I'm aware that Redmine in fact sometimes ignores invalid parameters rather than rejecting them, but I believe this should be restricted.

In case we still want to use the second patch, I think the code could be simplified to this with the exact same semantics:

@author = User.visible.active.find_by_id(params[:user_id].to_i)

Holger Just wrote:

Michael Vorobiev: Thank you for finding this and providing the patches!

No problem. Off-topic: Fun fact is that I actually stumbled upon this (blatantly obvious, once identified) issue while reviewing the implementation of #33602 for potential leaks exactly like this one...

Holger Just wrote:

I think it is clearer to explicitly deny access rather than silently ignoring an explicit parameter. As such, I prefer the first patch. I'm aware that Redmine in fact sometimes ignores invalid parameters rather than rejecting them, but I believe this should be restricted.

I tend to lean towards that opinion too.

In case we still want to use the second patch, I think the code could be simplified to this with the exact same semantics [...]

FWIW: I just tried to provide code that was as explicit as possible. Nevertheless: that line looks nice and tidy. Gotta like Ruby for such...

Thanks Mischa for catching this!

Holger Just wrote:

I think it is clearer to explicitly deny access rather than silently ignoring an explicit parameter. As such, I prefer the first patch. I'm aware that Redmine in fact sometimes ignores invalid parameters rather than rejecting them, but I believe this should be restricted.

• opening the issue url (/issues/:issue_id) returns 403 which is correct.
• filtering the issues list after issue id (issues?v[issue_id][]=issue_id) returns "No data to display" which is more friendly (from my perspective).

Also, I played with some filters from Github/Gitlab and they have the same behaviour, showing messages like "No results found" if you enter invalid params.

In case we still want to use the second patch, I think the code could be simplified to this with the exact same semantics:

[...]

Good ideea, thanks!

Marius BALTEANU wrote:

I'm in favour of keeping the existing behaviour from filters where the filter value is ignored. Lets take a case where a user don't have access to an issue:

• opening the issue url (/issues/:issue_id) returns 403 which is correct.
• filtering the issues list after issue id (issues?v[issue_id][]=issue_id) returns "No data to display" which is more friendly (from my perspective).

This is not entirely the same situation. In the issue filter example you describe, Redmine "pretends" the issue does not exist but still applies the filter.

In the case at hand however, if we ignore the filter (i.e. option 2), the behavior would be the same as if no filter would have been supplied in the request at all, thus showing the normal activity view of all users, not just for the given user id. In my eyes, this would be rather surprising and confusing.

Following your logic, the behavior should be correct if we either:

• show a 404, i.e. show that there is no data for the given filter, or
• to return an empty activity list with a 200, i.e., pretend the user has no activities (whether that is true or not)

The option to return an empty results set however makes it impossible to distinguish between an invalid filter and a valid one which just returns an empty set. We would also have to make sure that we are still not leaking any data of the hidden author.

As such, I'm still in favor of option 1 as it makes it explicit that the whole filter is invalid. This allows to distinguish it from a valid query which returns either an empty results set (if we return an empty activity list for an invalid user) or the result set for an entirely different request (with option 2). In my eyes, option 1 is the easiest option to implement with the fewest corner cases.

Thanks Holger for your clarifications, I've committed the first patch.

Should we release new versions?

Marius BALTEANU wrote:

Should we release new versions?

Sure. You are planning to release 4.1.5 and 4.2.3, right? I would then request a CVE entry for this.

In the security scanner, I would mark this issue as moderate severity, given that this circumvents the user visibility restrictions in many cases. Do you agree?

Holger Just wrote:

Marius BALTEANU wrote:

Should we release new versions?

Sure. You are planning to release 4.1.5 and 4.2.3, right? I would then request a CVE entry for this.

In the security scanner, I would mark this issue as moderate severity, given that this circumvents the user visibility restrictions in many cases. Do you agree?

Sorry for my late reply. Yes, I'm thinking to release 4.1.5 and 4.2.3 next weekend (October 10th). Regarding the severity, yes, it sounds good to me.

I've updated the Security_Advisories. Holger, feel free to update based on the security scanner.

Thanks for the release. I have updated the security scanner with the new releases.

On Monday, I'll request a CVE entry for this. Once it's assigned, I'll update the Security_Advisories wiki page and the Security scanner accordingly.

CVE-2021-42326 was assigned for this issue.