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-20179: Fix renewal profile approval process - v10.9 #3476

Merged
merged 1 commit into from Mar 15, 2021

Conversation

cipherboy
Copy link
Member

Due to a recent change in PKI CLI, the CLI now passes along user
authentication with submissions to the renewal endpoint. Unlike the EE
pages, the REST API has passed along this authentication for a while.
Due to a bug in the RenewalProcessor, requests with credentials against
profiles with no authentication method and no ACLs result in the
certificiate automatically being approved. This occurs because, when
an earlier commit (cb9eb96) modified
the code to allow Light-Weight SubCAs to issue certificates, validation
wasn't done on the passed principal, to see if it was a trusted agent.
Because profiles requiring Agent approval have an empty ACL list (as, no
user should be able to submit a certificate request and have it
automatically signed without agent approval), authorize allows any user
to approve this request and thus accepts the AuthToken.

Critical analysis: the RenewalProcessor code interprets (authToken
!= null) as evidence that the authenticated user is authorized to
immediately issue the certificate. This mismatch of concerns (authn
vs authz) resulted in a misunderstanding of system behaviour. The
"latent" AuthToken (from the HTTP request) was assigned to authToken
without realising that authorization needed to be performed.

We fix this by splitting the logic on whether the profile defines an
authenticator. If so, we (re)authenticate and authorize the user
according to the profile configuration.

If the profile does not define an authenticator but there is a
principal in the HTTP request, if (and only if) the user has
permission to approve certificate requests and the requested
renewal profile is caManualRenewal (which is hardcoded to be used
for LWCA renewal), then we issue the certificate immediately. This
special case ensures that LWCA renewal keeps working.

Otherwise, if there is no principal in the HTTP request or the
principal does not have permission to approve certificate requests,
we leave the authToken unset. The resulting renewal request will be
created with status PENDING, i.e. enqueued for agent review.

Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
Signed-off-by: Alexander Scheel <ascheel@redhat.com>

References: CVE-2021-20179
Resolves: rh-bz#1914379

Due to a recent change in PKI CLI, the CLI now passes along user
authentication with submissions to the renewal endpoint. Unlike the EE
pages, the REST API has passed along this authentication for a while.
Due to a bug in the RenewalProcessor, requests with credentials against
profiles with no authentication method and no ACLs result in the
certificiate automatically being approved. This occurs because, when
an earlier commit (cb9eb96) modified
the code to allow Light-Weight SubCAs to issue certificates, validation
wasn't done on the passed principal, to see if it was a trusted agent.
Because profiles requring Agent approval have an empty ACL list (as, no
user should be able to submit a certificate request and have it
automatically signed without agent approval), authorize allows any user
to approve this request and thus accepts the AuthToken.

Critical analysis: the RenewalProcessor code interprets (authToken
!= null) as evidence that the authenticated user is /authorized/ to
immediately issue the certificate.  This mismatch of concerns (authn
vs authz) resulted in a misunderstanding of system behaviour.  The
"latent" AuthToken (from the HTTP request) was assigned to authToken
without realising that authorization needed to be performed.

We fix this by splitting the logic on whether the profile defines an
authenticator.  If so, we (re)authenticate and authorize the user
according to the profile configuration.

If the profile does not define an authenticator but there is a
principal in the HTTP request, if (and only if) the user has
permission to approve certificate requests *and* the requested
renewal profile is caManualRenewal (which is hardcoded to be used
for LWCA renewal), then we issue the certificate immediately.  This
special case ensures that LWCA renewal keeps working.

Otherwise, if there is no principal in the HTTP request or the
principal does not have permission to approve certificate requests,
we leave the authToken unset.  The resulting renewal request will be
created with status PENDING, i.e. enqueued for agent review.

Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
Signed-off-by: Alexander Scheel <ascheel@redhat.com>
@cipherboy cipherboy added Bug Bug fixes Backport Already existing patch to be included in different release labels Mar 12, 2021
@cipherboy cipherboy self-assigned this Mar 12, 2021
Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch looks good, but for some reason the tests are failing. I suppose it's caused by a different issue. Let me investigate this, and also wait for @ladycfu if she has any other comment on this patch, then I'll merge the patch for all branches. Thanks!

Copy link
Contributor

@ladycfu ladycfu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks same as I last reviewed and tested.

@edewata
Copy link
Contributor

edewata commented Mar 15, 2021

The tests are failing due to a DS issue that has been fixed in v10.9 branch, so I think we can merge this PR safely. The IPA test will still fail since IPA is using PKI 10.10 on all Fedora platforms now. We might just drop the IPA test from v10.9. That can be done separately later.

@edewata edewata merged commit 1204a43 into dogtagpki:v10.9 Mar 15, 2021
8 of 18 checks passed
@cipherboy
Copy link
Member Author

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport Already existing patch to be included in different release Bug Bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants