Register table URI encoding

[mike-gormley]

Hello,

It's taken me a few weeks to come back to this but following on from #181 we've encountered a further issue. Seems I only did affirmative testing against the previous issues raised rather than further regression testing.

We've found that the link in the name column of the registers is 404'ing due to the URL encoding of the register/entity path. For example, https://mywebsite.com/_my-register is becoming https://mywebsite.com/%2F_my-register, or https://mywebsite.com/my-register/_sub-register is becoming https://mywebsite.com/%2Fmy-register%2Fsub-register

I'm uncertain if this is something caused by our template customisations or whether this occurs in the standard templates. Could anyone confirm that version 2.3.14 is working (or not working) as expected?

[der]

Hi Mike,

You are right, there we're knock on changes needed which we failed to spot. The linked PR works locally, can you verify?

In passing I see that the changes for #181 involved a typo but inclined to leave that to a future clean up.

[mike-gormley]

Hi @der, thank you for looking into this. I've deployed the changes to a development environment and it resolves the register table links. However, it reverses the original XSS issue in #181.

We've also learned we can add a URL to the Login modal close button return querystring parameter and it is even possible to add a URL e.g. /ui/login?return=https://www.google.co.uk, so not just javascript.

We've also found another location at ./?entity=/Some-text-here.

[der]

Hi @der, thank you for looking into this. I've deployed the changes to a development environment and it resolves the register table links. However, it reverses the original XSS issue in #181.

Sorry about that, that was careless. I was sure I had tested that case.

The regression should be reversed by UKGovLD/registry-config-base#37

We've also learned we can add a URL to the Login modal close button return querystring parameter is even possible to add a URL e.g. /ui/login?return=https://www.google.co.uk, so not just javascript.

Switching back to the correct xss clean for the close button case should have addressed this as well.

We've also found another location at ./?entity=/Some-text-here.

That was an interesting case. It's supposed to be able show what it knows about external entities but the underlying check was returning empty information instead of an explicit 404 so the render chosen was incorrect. That should be fixed too.

Please let me know if these changes work for you.

[mike-gormley]

I think we're nearly there. I've deployed another instance today with the changes in UKGovLD/registry-config-base#37 and been testing what I can. On the whole, looks like it's handling many of the scenarios previously mentioned as well as it can though I think the entity param issue resolution is at odds with the intended purpose as you refer to.

?entity=http://www.mybadwebsite.com now shows a 'Not found (404)', which is want we want.

Conversely, the intended use of this query parameter was to also cater for external definition entities such as /?entity=http://www.w3.org/2004/02/skos/core%23Concept now doesn't work as intended. Use of this in the templates (e.g. The type in the metadata pane of _item-render.vm might get around this by directly linking to those known external URIs (as it does elsewhere) but you might know of other ways to address this.

Unfortunately, I have found that the 404 page also suffers from the same problem as what we are trying to resolve. It will display the invalid URL in the page as a clickable link, which can actually be valid in this format!

We could fix this in the _not-found.vm template ourselves by removing the parroting of the invalid URL as an interactive link (or preferably not mentioned at all) but would be helpful if we didn't need to maintain a divergence in functionality in our own instances.

The issue is compounded by a separate concern in that 404 pages actually return HTTP 200 OK responses so has potential to be treated as valid pages of our websites and indexed by web bots (that link to web pages outside our control).

[marqh]

The issue is compounded by a separate concern in that 404 pages actually return HTTP 200 OK response

this seems quite crucial to me
i'd expect to receive an HTTP 404 from such a page, as well as a rendered not found page

where is the HTTP response code managed in these cases?

would it be possible to include the correct HTTP response code to ensure that the ?entity pattern can be coded against, using HTTP codes to differentiate exists from does-not-exist

many thanks, mark

[der]

The ?entity queries at the UI level were supposed to be answering "what do you know about X" and a 200 response saying "I don't know anything about X" seems entirely the correct response. This is quite different from retrieving a URI within the registry namespace and getting a 404 if it doesn't exist.

[der]

Conversely, the intended use of this query parameter was to also cater for external definition entities such as /?entity=http://www.w3.org/2004/02/skos/core%23Concept now doesn't work as intended.

Well, actually your registry probably doesn't have any registered item for http://www.w3.org/2004/02/skos/core#Concept so it is now returning the "correct" response, it's just not a helpful one!

I think our issue is that the templates which render external entities are using the ?entity queries just in case there is additional information on them within the registry. I'm not sure there's a way we can do that without having this exposure. So the answer may be to stop generating any of these ?entity queries in the first place and simply link directly to the external URL. While leaving the entity queries in place to allowing anyone to check for registered external entities via this route if required.

Not sure how big a job that is but can try to take a look this evening.

Unfortunately, I have found that the 404 page also suffers from the same problem as what we are trying to resolve. It will display the invalid URL in the page as a clickable link, which can actually be valid in this format!

Yes, I wondered about that. While it's sanitised in the sense that it should be blocking e.g. javascript it does render a link. If that's not desirable, and I can see the argument against it, then can remove the href from the 404 template.

[der]

I've updated the PR with a proposed and fairly simple resolution.

This removes the use of ?entity calls internally. Now any external URI reference from a registered entry links directly to that external entity and there is no reliance on the ?entity UI functionality for these.

Secondly it removes the processing of the ?entity query parameter entirely from the UI. So a query to /entity=http://www.mybadwebsite.com is treated as simply a query to /. This is similar to how sites like https://www.google.com handle spurious query parameters.

Apart from the now-removed internal use, the only purpose of the ?entity query could be to search for external registered entries. At the UI level that is more conveniently done by using the check operation under the advanced menu which already handles non-registered entities without rendering a link to them.

This brute force removal of the functionality seems safer than relying on correct detection of unregistered entities, however we report them.

Please let me know if this is an acceptable solution.

[der]

I've also taken a pass through the code to identify other potential issues in the admin pages and added additional guards for those cases.

[der]

Many of the additional issues discovered occur in admin pages. I've also disabled all admin page templates when not authorized and taken a further pass over the partials that are used in such pages since it seems that if an attacker has access to the code they could try to invoke those directly.

This pass through the code is complete enough for review. I'm hopeful that we've now mitigated these vulnerabilities but would be grateful for any further checking you can do.

[mike-gormley]

@der You've been busy the last 24 hours! I really appreciate the broader approach to locating other potential loopholes in the templates.

Just to place an update here from my perspective at the end of progress today. I'd deployed the main chunk of last night's/this morning's commits to a local dev environment earlier today and you've seen I've added one or two comments on the more recent proposed changes already. The complete removal of the ?entity= is a welcome proposal, as are the additional validations in place, and the removal of the link on the Not Found page. Previous vulnerabilities appear to be addressed.

I'm still regression testing as much of the registry capabilities as I can find in slower time. The only remaining issue I've seen is the Close button on the login modal and the create-redirect modal forms both have stopped working as the URIs are encoded. I'm less fussed about this in the grand scheme of things and could be split out into a separate bugfix. The redirects for actually logging in/registering still function as intended and still cater for misbehaviour so not all bad here.

I'll do another deploy of all commits tomorrow to test the newest commits, and hopefully we can promote these changes. Thanks again.