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

As a developer, I want the Referrer-Policy in the response header from the tasker to be strict-origin-when-cross-origin #392

Closed
HankHerr-NOAA opened this issue Jan 30, 2025 · 37 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@HankHerr-NOAA
Copy link
Contributor

HankHerr-NOAA commented Jan 30, 2025

EDIT: Also, we want to add Content-Security-Policy: none to the response header and remove Server: Jetty(12.0.16) from the header if possible.

ORIGINAL DESCRIPTION:

See VLab ticket 142394. It identified a medium-risk vulnerability that the tasker was not returning a Referror-Policy in the response header. We need to add that to the header, and, based on review of the recommended fix in the document attached to that VLab ticket, ensure the value is set to strict-origin-when-cross-origin.

Thanks,

Hank

@HankHerr-NOAA HankHerr-NOAA added the bug Something isn't working label Jan 30, 2025
@HankHerr-NOAA HankHerr-NOAA added this to the v6.29 milestone Jan 30, 2025
@HankHerr-NOAA
Copy link
Contributor Author

I'd like to piggy back on this ticket for two more changes. We want to add Content-Security-Policy: none to the response header and remove Server: Jetty(12.0.16) from the header if possible.

Please handle all three of these changes in this ticket... whoever ends up addressing it.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

We're going to get more information on Content-Security-Policy. Google's AI is recommending heavily against just "none".

Hank

@HankHerr-NOAA
Copy link
Contributor Author

The code to change for all HTTP request headers is this in Tasker.java:

        ResourceHandler resourceHandler = new ResourceHandler()
        {
            @Override
            public boolean handle( Request request,
                                   Response response,
                                   Callback callback )
                    throws Exception
            {
                response.getHeaders()
                        .add( "X-Frame-Options", "DENY" );
                response.getHeaders()
                        .add( "strict-transport-security", "max-age=31536000; includeSubDomains; preload;" );
                return super.handle( request, response, callback );
            }
        };

Adding Referrer-Policy should be easy, as will be adding Content-Security-Policy once we have identified an appropriate policy that will pass the scan.

The response.getHeaders() returns just a Collection, so there is no straightforward way to remove the server header parameter. That might require a bit of work. Worth noting that my Eclipse is broken, at the moment, so I'm not really sure what commands are provided for navigating/replacing existing headers.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Finally able to get Eclipse to work, it appears that simply calling response.getHeaders().remove("Server") should work. However, this is calling the super.handle(...) method, so if that method is adding in the Server to the header, then removing it before its added won't do a thing. That will require testing.

Bottom line, pending a value for Content-Security-Policy, the following should work:

        //Static handler for index.html. This also impacts the dynamic resources
        //by being chained in setHandler, below.
        ResourceHandler resourceHandler = new ResourceHandler()
        {
            @Override
            public boolean handle( Request request,
                                   Response response,
                                   Callback callback )
                    throws Exception
            {
                response.getHeaders()
                        .add( "X-Frame-Options", "DENY" );
                response.getHeaders()
                        .add( "strict-transport-security", "max-age=31536000; includeSubDomains; preload;" );
                response.getHeaders()
                        .add( "Content-Security-Policy", "TBD");
                response.getHeaders()
                        .add( "Referrer-Policy", "strict-origin-when-cross-origin" );
                response.getHeaders()
                        .remove(  "Server" );
                return super.handle( request, response, callback );
            }
        };

As James said in the meeting, handling this through configuration would be preferred, but I'm not sure we have time to work that out right now. The above would require testing in -dev and -ti, obviously, before being sure it will work.

Hank

@HankHerr-NOAA HankHerr-NOAA self-assigned this Jan 31, 2025
@HankHerr-NOAA
Copy link
Contributor Author

I'm giving this one a shot. For testing purposes, I'm going to use Content-Security-Policy of none. First, I need to install what I have in development. Unfortunately, I need to update the OpenJDK version in the Dockerfiles and maybe other dependencies, and, every time I need to do this, I have to remind myself how.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

OpenJDK and unzip needed updating. I removed the versions for the purposes of this ticket after my first attempt to update (using --progress plain for guidance) failed likely due to user error. With those version removed, and with artemis upped to 2.39.0 in the events broker Dockerfile, the COWRES was able to deploy to to the -dev entry machine. A smoke test passed.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Here are the headers currently returned when calling a GET for job:

< Server: Jetty(12.0.16)
< Date: Fri, 31 Jan 2025 13:31:51 GMT
< X-Frame-Options: DENY
< Strict-Transport-Security: max-age=31536000; includeSubDomains; preload;
< Content-Type: text/plain;charset=utf-8
< Vary: Accept-Encoding
< Content-Length: 2

I'm going to make the change to the tasker, deploy, and see what happens,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

The attempt to remove Server is resulting in this exception:

java.lang.UnsupportedOperationException: Persistent field
        at org.eclipse.jetty.server.internal.ResponseHttpFields$2.remove(ResponseHttpFields.java:216)
        at org.eclipse.jetty.http.HttpFields$Mutable.remove(HttpFields.java:1529)
        at wres.tasker.Tasker$1.handle(Tasker.java:182)
        at org.eclipse.jetty.server.Server.handle(Server.java:182)
        at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:662)
        at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:418)
        at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
        at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
        at org.eclipse.jetty.io.ssl.SslConnection$SslEndPoint.onFillable(SslConnection.java:575)
        at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:390)
        at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:150)
        at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
        at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979)
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209)
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164)
        at java.base/java.lang.Thread.run(Thread.java:840)

Need to step away and then have a meeting with Jeff. I'll try to figure out what to do about the Server when I return.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

As expected, if I add an empty Server, it does not replace the existing one. I'm going to do some web searching to see if there is a way to remove the Server from the request headers spelled out somewhere online,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Found this:

https://stackoverflow.com/questions/15652902/remove-the-http-server-header-in-jetty-9

Code snippet:

HttpConfiguration httpConfig = new HttpConfiguration();
httpConfig.setSendServerVersion( false );
HttpConnectionFactory httpFactory = new HttpConnectionFactory( httpConfig );
ServerConnector httpConnector = new ServerConnector( server,httpFactory );
server.setConnectors( new Connector[] { httpConnector } );

I need to figure out where the call to setSendServerVersion could be inserted into our code.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

I added the call to setSendServerVersion just after our call to new HttpConfiguration() in Tasker.java:

        HttpConfiguration httpConfig = new HttpConfiguration();

        // Remover Server from the response headers.
        httpConfig.setSendServerVersion( false );

The header is now:

< HTTP/1.1 200 OK
< Date: Fri, 31 Jan 2025 14:23:53 GMT
< X-Frame-Options: DENY
< Strict-Transport-Security: max-age=31536000; includeSubDomains; preload;
< Content-Security-Policy: none
< Referrer-Policy: strict-origin-when-cross-origin
< Content-Type: text/plain;charset=utf-8
< Vary: Accept-Encoding
< Content-Length: 2

Last thing to figure out is what to set the Content-Type to, preferably something that will pass a scan. Arvin proposed a very long, detailed one for the WRES GUI yesterday that made it so that the WRES GUI could not show an image in the GUI. Since we don't have images to display (our interface is raw), I'll see if his works for us.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Here is a Python code snippet Arvin provided:

add_header Content-Security-Policy "
    default-src 'self' https: data: blob:;
    script-src 'self' 'unsafe-inline' 'unsafe-eval' https: data: blob:;
    style-src 'self' 'unsafe-inline' https: data: blob:;
    img-src 'self' data: https:;
    font-src 'self' data:;
    connect-src 'self' https:;
    object-src 'none';
" always;

I'm going to use that in the tasker,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Here is the header now:

< HTTP/1.1 200 OK
< Date: Fri, 31 Jan 2025 14:36:22 GMT
< X-Frame-Options: DENY
< Strict-Transport-Security: max-age=31536000; includeSubDomains; preload;
< Content-Security-Policy: default-src 'self' https: data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval' https: data: blob:; style-src 'self' 'unsafe-inline' https: data: blob:; img-src 'self' data: https:; font-src 'self' data:; connect-src 'self' https:; object-src 'none';
< Referrer-Policy: strict-origin-when-cross-origin
< Content-Type: text/plain;charset=utf-8
< Vary: Accept-Encoding
< Content-Length: 2

I'm going to check various aspects of the COWRES to make sure everything still works,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Everything looks good. I was able to post a smoke test, check its status, check stdout, list and download output, visit the broker, and see the expected number of connections and queues. Only the tasker responses should be affected, but I figured I would look at nearly everything.

Bottom line, I think we are good here. I'm going to create a pull request and get this code into master. Here are all of the changes to Tasker.java. If anyone sees an issue, please let me know:

        //Static handler for index.html. This also impacts the dynamic resources
        //by being chained in setHandler, below.
        ResourceHandler resourceHandler = new ResourceHandler()
        {
            @Override
            public boolean handle( Request request,
                                   Response response,
                                   Callback callback )
                    throws Exception
            {
                response.getHeaders()
                        .add( "X-Frame-Options", "DENY" );
                response.getHeaders()
                        .add( "strict-transport-security", "max-age=31536000; includeSubDomains; preload;" );
                response.getHeaders()
                        .add( "Content-Security-Policy", "default-src 'self' https: data: blob:;"
                                + " script-src 'self' 'unsafe-inline' 'unsafe-eval' https: data: blob:;"
                                + " style-src 'self' 'unsafe-inline' https: data: blob:;"
                                + " img-src 'self' data: https:;"
                                + " font-src 'self' data:;"
                                + " connect-src 'self' https:;"
                                + " object-src 'none';");
                response.getHeaders()
                        .add( "Referrer-Policy", "strict-origin-when-cross-origin" );
                return super.handle( request, response, callback );
            }
        };

... SNIP ...

        HttpConfiguration httpConfig = new HttpConfiguration();

        // Remover Server from the response headers.
        httpConfig.setSendServerVersion( false );

Thanks,

Hank

HankHerr-NOAA added a commit that referenced this issue Jan 31, 2025
Fixing vulnerabilities; refs GitHub #392
@HankHerr-NOAA
Copy link
Contributor Author

Changes are in. Hopefully, I did that right.

Closing and marking for UAT,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

I'm reopening this issue. To test this change, I need it in staging. To get it in staging, I need to update the Dockerfiles with the new OpenJDK and unzip. I tried to make that change earlier and failed. Let me try it again.

Once I have the Dockerfiles updated, I'll do another PR/merge. This will be a minor dependency updates solely to get it working, which is why I don't want to create a full blown dependency update ticket. If anyone has any concerns about that approach, let me konw.

Thanks,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

I've got the correct versions now in the Dockerfiles. Here is the old and new as reported by git diff:

-    java-17-openjdk-headless-1:17.0.13.0.11-3.el8.x86_64 \
-    unzip-6.0-46.el8 \
+    java-17-openjdk-headless-1:17.0.14.0.7-3.el8.x86_64 \
+    unzip-6.0-47.el8_10.x86_64 \

Images appear to build, but I can't deploy to development at the moment due to a long running evaluation. It should be safe enough, however, to push the Dockerfiles to master, I think. You can see how the version have changed and these are the versions that were used in development (just not spelled out explicitly in the Dockerfiles).

I'm going to prep and pull request and merge to master,

Hank

HankHerr-NOAA added a commit that referenced this issue Jan 31, 2025
Updated Dockerfiles for openjdk and unzip; refs GitHub #392
@HankHerr-NOAA
Copy link
Contributor Author

The merge completed. I need to no force a deploy to staging so that I can double check if the header parameters look right through the proxy.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

After some difficulties, I was able to get the COWRES deployed to staging and confirmed that the headers look fine when accessed via internal URL or proxy.

Closing this ticket once again and marking ready for UAT.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Just deleted a partial comment.

The scanner, when pointed at the staging proxy, complained about the script-src and style-src options of the content security policy for the WRES GUI. Though it did not complain about the COWRES, perhaps because this parameters don't apply since its not a web interface (???), I'm still going to remove the unsafe values for those two options and see what happens. I'll do that this morning and test in development.

Thanks,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

The change is in development and the -dev COWRES looks good. I'll create the branch and pull request now.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

The pull request is still in process, but I deployed the tasker image to staging. What I thought was a fluke was actually real: modifying the Content-Security-Policy results in the size of the text area for entering the declaration into the front-end (the text area that defaults to the smoke test) to be incorrect. Its way too small.

So something in the display of the front-end API is getting screwed up by the change. I'll do a bit more research on Script-Src and Style-Src to see if I can figure it out.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

So our front-end API requires unsafe-inline in order to appear correct via browser.

Is there any way to adjust the index.html in order to make the front-end API appear correct without the unsafe-inline Content-Security-Policy option?

Still investigating,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Next attempt: manually specifying the number of columns and rows for the text area.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Nope. Adding rows and cols does nothing.

Hank

@HankHerr-NOAA
Copy link
Contributor Author

So this seems to work:

    <textarea id="projectConfig" class="evaluation" name="projectConfig" required="true" style="resize:both;" rows="40" cols="80">

I had to add the style and the dimensions. Does anyone have any concerns about fixing the size of the text area to be 40 rows and 80 columns before scrolling happens? Those dimension were pretty close to the default when using unsafe-inline.

I need to step away. If no one sees any issues with that, I'll create the pull request and merge when I return. Thanks,

Hank

@james-d-brown
Copy link
Collaborator

You could try in a couple of browsers and screen configurations, but I don't see this being an issue.

It's not that important, anyway, as it's just the landing page for the backend. There may be some folks using this as a convenience, but it isn't the entry point for software, which is the main purpose of the backend.

@james-d-brown
Copy link
Collaborator

As an aside, you can keep a PR open indefinitely, pushing incrementally as normal - it's really just a marker of intent and you can start one when you start a new ticket/activity that encompasses many future pushes.

@HankHerr-NOAA
Copy link
Contributor Author

Thanks, James, for that piece of information. I'll try to reopen the previous pull request, then, if that's a thing. If so, I'll push the index.html change. I'm not sure if that will trigger another round of tests (since its unrelated to those tests), but I'm guessing so since it should trigger a new set of .zips.

I'll test the API in the two browsers to which I have access and confirm it looks okay in both. Testing the change in staging now,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

When the cache is cleared, the text area looks fine and the COWRES works fine based on smoke tests. This was tested using staging. I'll leave it in staging so that the WRES GUI can post some scheduled evaluations as further confirmation that everything is okay.

Adding the index.html change to the pull request,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

The pull request picked up the change immediately and tests are running again. If all goes well, I'll merge,

Hank

HankHerr-NOAA added a commit that referenced this issue Feb 3, 2025
Removed unsafe options from content security policy; refs Github #392
@HankHerr-NOAA
Copy link
Contributor Author

I think that marks the end of the work for COWRES, at least based on a scan using staging. In fact, the scanner didn't even pickup on the problem with the COWRES endpoint, so I only made the changes to Content-Security-Policy "just in case".

Closing this out and then moving it to UAT, just a reminder to check and make sure all scheduled evaluations through the staging COWRES and any UAT during deployment all succeed.

Thanks,

Hank

@HankHerr-NOAA
Copy link
Contributor Author

Evan:

I ask that you perform the UAT for this one in staging. You should see the following changes if you curl it:

  1. No Server in the response header.
  2. Referrer-Policy of strict-origin-when-cross-origin
  3. Content-Security-Policy does not include any unsafe-* values.
  4. The text area should be appropriately sized when visiting the "front end".

If you would like the exact value to expect in for the CSP, let me know. It basically covers everything an only uses self, iirc.

Thanks,

Hank

@epag
Copy link
Collaborator

epag commented Feb 6, 2025

I don't think this is blocking, but noticed that the screen we have for checking validity of declarations does shrink (The main page text area looks fine though)

Image

@epag
Copy link
Collaborator

epag commented Feb 6, 2025

Think this looks good otherwise:

< HTTP/1.1 200 OK
< Date: Thu, 06 Feb 2025 21:01:18 GMT
< X-Frame-Options: DENY
< Strict-Transport-Security: max-age=31536000; includeSubDomains; preload;
< Content-Security-Policy: default-src 'self' https: data: blob:; script-src 'self' https: data: blob:; style-src 'self' https: data: blob:; img-src 'self' data: https:; font-src 'self' data:; connect-src 'self' https:; object-src 'none';
< Referrer-Policy: strict-origin-when-cross-origin
< Content-Type: text/plain;charset=utf-8
< Vary: Accept-Encoding
< Content-Length: 2

@HankHerr-NOAA
Copy link
Contributor Author

I forgot about that page. We should create another ticket for that fix, and it should just require a change to its HTML matching what I did for the front-end HTML.

If you don't do this today, I'll do it tomorrow (unless my memory fails me).

Hank

@epag
Copy link
Collaborator

epag commented Feb 6, 2025

Created the ticket for it

#407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants