Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVE-2021-41033: Provide improved support for dealing with http protocol in p2 #230

Closed
merks opened this issue Mar 21, 2023 · 14 comments
Closed

Comments

@merks
Copy link
Contributor

merks commented Mar 21, 2023

Improve the handling of http and ftp protocol by the p2 framework.

The following links provide background detail:

Some work has been done to log warnings when the use of http protocol is detected in some limited contexts:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=576428

Such warnings were considered undesirable in some environments, e.g., inside a corporate firewall, so support was provided to allow logged warnings to be disabled:

#123

To provide both maximum flexibility and maximum security, we expand the current approach to fully address CVE-2021-41033 and beyond. This description below has been edited to reflect the final design.

We focus on guarding/monitoring p2's transport layer as a whole such that all internet access is taken into consideration, including the following mainstream use cases:

There are three modes of http/ftp handling:

  1. allow -Dp2.httpRule=allow -Dp2.ftpRule=allow
  • This is basically what we have now, i.e., log a warning but allow that warning to be suppressed via p2.skipRepositoryProtocolCheck. The warning will be logged for all http/ftp access, not simply once per repository and only for repository URLs.
  1. redirect -Dp2.httpRule=redirect -Dp2.ftpRule=redirect
  • Any http/ftp URI will be redirected/written to use https/ftps such that no http/ftp request ever reaches out into the network.
  1. block -Dp2.httpRule=block -Dp2.ftpRule=block
  • Any http/ftp URI will be blocked as if the network were down/inaccessible.

One important question is "what should be the default behavior in the absence of an explicit choice of one of those three"? I propose the framework defaults to block http access. Product/application builders (or even users) can then choose whether to allow such access or redirect such access; they will be alerted to that choice by the exception that blocks the access. (Note that within a corporate firewall, it might well be the case that redirection to https causes failures due to missing root certificates so allow, while not ideal, may be needed in some limited environments where security is ensured via external mechanisms.) In the end though, we decided to make redirect the default because this is assumed to be what most people will want.

Comments on the proposal are welcome!

@laeubi
Copy link
Member

laeubi commented Mar 21, 2023

Any https URI will be blocked as if the network were down/inaccessible.

Should it not the other way round ;-)

Maven is blocking http for a while and even though it produce some disturbance it seems people can manage this, so block as a default seems suitable to me.

redirect

Probably better "rewrite" as redirection would need to happen on the server side, this seems similar to the "http-only" setting in firefox: https://support.mozilla.org/en-US/kb/https-only-prefs

@merks
Copy link
Contributor Author

merks commented Mar 21, 2023

Oops. 😜

I edited the original text of fix the for block!

I personally prefer the term redirect because "rewrite" sounds a little more like we might be modify (i.e., literally re-write) the sources of the URLs when we are actually "directing" the underlying transport layer to use https no matter what. The https-only link doesn't use either term. This is more about style/preference rather than substance, so if someone else feels strongly, I have bigger fish to fry. 😛 In the implementation this is controlled by system properties...

@laeubi
Copy link
Member

laeubi commented Mar 21, 2023

Well on the transport layer it is actually enforce SSL :-D

Maybe it would then be better to not use such naming as allow / block / ... so maybe more make it more generic as its done with cipher suites to have a blacklist of protocols (!), as for example one might also thing that plain ftp is bad, so maybe like this as a default (block):

-Dp2.transport.insecure = warn
-Dp2.transport.block = ftp,http

to allow but warn:

-Dp2.transport.block = 

to allow but do not warn:

-Dp2.transport.insecure = debug
-Dp2.transport.block = 

And if one really wants redirect

-Dp2.transport.http = redirect

merks added a commit to merks/p2 that referenced this issue Mar 22, 2023
Support three modes of http handling:

- allow
  - This is basically what we have now, i.e., log a warning but allow
that warning to be suppressed.  The warning will be logged for all http
access, not simply once per repository and only for repository URLs.
- redirect
  - Any http URI will be redirected/written to use https such that no
http request ever reaches out into the network.
- block
  - Any http URI will be blocked as if the network were
down/inaccessible.

The default behavior is block.

The system property p2.httpAction can be used to configure the behavior.

eclipse-equinox#230
merks added a commit to merks/p2 that referenced this issue Mar 22, 2023
Support three modes of http handling:

- allow
  - This is basically what we have now, i.e., log a warning but allow
that warning to be suppressed.  The warning will be logged for all http
access, not simply once per repository and only for repository URLs.
- redirect
  - Any http URI will be redirected/written to use https such that no
http request ever reaches out into the network.
- block
  - Any http URI will be blocked as if the network were
down/inaccessible.

The default behavior is block.

The system property p2.httpAction can be used to configure the behavior.

eclipse-equinox#230
merks added a commit to merks/p2 that referenced this issue Mar 22, 2023
Support three modes of http handling:

- allow
  - This is basically what we have now, i.e., log a warning but allow
that warning to be suppressed.  The warning will be logged for all http
access, not simply once per repository and only for repository URLs.
- redirect
  - Any http URI will be redirected/written to use https such that no
http request ever reaches out into the network.
- block
  - Any http URI will be blocked as if the network were
down/inaccessible.

The default behavior is block.

The system property p2.httpAction can be used to configure the behavior.

eclipse-equinox#230
merks added a commit that referenced this issue Mar 22, 2023
Support three modes of http handling:

- allow
  - This is basically what we have now, i.e., log a warning but allow
that warning to be suppressed.  The warning will be logged for all http
access, not simply once per repository and only for repository URLs.
- redirect
  - Any http URI will be redirected/written to use https such that no
http request ever reaches out into the network.
- block
  - Any http URI will be blocked as if the network were
down/inaccessible.

The default behavior is block.

The system property p2.httpAction can be used to configure the behavior.

#230
@merks merks closed this as completed Mar 22, 2023
@merks
Copy link
Contributor Author

merks commented Mar 22, 2023

@akurtakov

Do you guys want the SDK product to specify -Dp2.httpAction=redirect?

@jonahgraham

Do you want the EPP products to specify -Dp2.httpAction=redirect?

I plan to do this for the Eclipse Installer...

@akurtakov
Copy link
Member

@akurtakov

Do you guys want the SDK product to specify -Dp2.httpAction=redirect?

I even think this should be the default value for the parameter if nothing else specified.

@jonahgraham

Do you want the EPP products to specify -Dp2.httpAction=redirect?

I plan to do this for the Eclipse Installer...

@merks
Copy link
Contributor Author

merks commented Mar 22, 2023

One "vague" reason I had for choosing block as the default is because this forces people to recognize/notice there is a decision to be made. But I suppose a counter argument is that if 99% of the applications (I made that number up) will want redirect, then make that the default. If the SDK, all the EPP packages, and the installer all want to specify redirect, it's certainly easier to make that the default.

So if @jonahgraham agrees and no one else objects strongly (with good arguments), I'm happy to change the default...

@iloveeclipse Maybe you have an opinion about the default?

@iloveeclipse
Copy link
Member

I'm not sure what the effect of "redirect" will be if server doesn't support https - that would lead to an error, right (and this will be OK for me)?

I would assume "redirect" would be a good default because most of proper configured update sites support https, and if not, they could switch to do so and no changes will be needed on user side, so all the "old" update sites that could have been in use will still work without users being forced to do anything.

@merks
Copy link
Contributor Author

merks commented Mar 22, 2023

Yes, if the server doesn't support https, there will be an error to that effect. Also, within corporate firewalls, there could be failures because of missing root certificates, which folks sometime avoid by using http, but since most things (touch points) are already specifying https, that's already often an issue.

@jonahgraham
Copy link
Contributor

redirect as a default is fine by me.

Probably should change the UI to default to https:// too:

image

@laeubi
Copy link
Member

laeubi commented Mar 23, 2023

One "vague" reason I had for choosing block as the default is because this forces people to recognize/notice there is a decision to be made.

Yes I think this is good, because even today I see a lot of "unsave http" warnings but no one seems to care!
Making it default redirect seems not to improve the situation here, so I think it should really not be the default.

Sadly the current implementation is only on the transport level otherwise I could think about something like even asking the user the same way as we ask for certificates:

  • Skip the mirror / edit the location
  • Try https instead
  • use http even if its unsecure

At least I think (depending on the setting) P2 might want to filter out unsecure mirrors and somehow notify them that they have to upgrade to https, e.g. by getting a list of mirrors that currently use unsafe transports.

For the UI, I think there should be a Warning whenever there is a site added with http and ask the user:

  1. Try https instead
  2. cancel the operation

It would therefore also be good to ask on start if one wants to convert all http -> https ...

merks added a commit to merks/p2 that referenced this issue Mar 23, 2023
Deal with both ftp and http protocol. The system property
p2.{protocol}Rule can be used to configure the behavior.

eclipse-equinox#230
@merks
Copy link
Contributor Author

merks commented Mar 23, 2023

It's clear I can't make everyone happy all of the time and it's clear that I do not have time to heat up nor cool down oceans. Prompting users about the protocol is just beyond the scope of what I plan to do. Such URLs can first appear within composites, or from references to other sites, i.e., places where the user just doesn't see where the URL comes from nor can the user change it...

I will change the default to minimize the amount of effort needed by product/application builders and I will address some of the concerns in via:

#234

As such , both -Dp2.ftpRule and -Dp2httpRule are supported and both are redirect by default.

I will not move the logic outside of the transport layer because it's way too likely some caller will overlook:
image

Also , the current implementation is hard coded so there is currently no use case. Should such a use case arise in the future, it can be addressed at that time.

@laeubi
Copy link
Member

laeubi commented Mar 23, 2023

You asked for feedback here

Comments on the proposal are welcome!

So I gave my feedback on where I think things might be improved, obviously you are free to ignore them, adapt how you seem best fit or whatever else, so for me its not about making anyone happy ...

Also , the current implementation is hard coded so there is currently no use case. Should such a use case arise in the future, it can be addressed at that time.

I think the use-case is already described here (even though that's not necessarily my use case or the eclipse use case), lets say I have one site that do not supports https (for whatever reason), this will currently fail unless I allow unsecure access for every site.

@merks
Copy link
Contributor Author

merks commented Mar 23, 2023

Your feedback has been helpful and constructive and has been adapted into the implementation to make it better and more general than it would have been without your feedback. 😄

merks added a commit that referenced this issue Mar 23, 2023
Deal with both ftp and http protocol. The system property p2.{protocol}Rule can be used to configure the behavior.

#230
@merks
Copy link
Contributor Author

merks commented Mar 24, 2023

Probably should change the UI to default to https:// too:

image

FYI, that was changed during the previous release cycle by this commit:

0dfc4b4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants