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

Jump to bottom

x/crypto/acme/autocert: sometimes uses AuthzURLs slice from client.WaitOrder rather than client.AuthorizeOrder #35225

Closed
dmitshur opened this issue Oct 29, 2019 · 1 comment
Closed

x/crypto/acme/autocert: sometimes uses AuthzURLs slice from client.WaitOrder rather than client.AuthorizeOrder #35225

dmitshur opened this issue Oct 29, 2019 · 1 comment
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

In CL 199520, a new Manager.verifyRFC method was added. It has a deferred function call inside a for loop:

o, err := client.AuthorizeOrder(ctx, acme.DomainIDs(domain))
if err != nil {
    return nil, err
}
// Remove all hanging authorizations to reduce rate limit quotas
// after we're done.
defer func() {
    go m.deactivatePendingAuthz(o.AuthzURLs)
}()

// ... a bunch of code

// All authorizations are satisfied.
// Wait for the CA to update the order status.
o, err = client.WaitOrder(ctx, o.URI)
if err != nil {
    continue AuthorizeOrderLoop
}
return o, nil

The variable o is captured by the deferred closure. The results of calling client.WaitOrder are assigned to o and err, which changes the value of o that the deferred closure uses. The variable o can become nil, which would cause a panic. It can be non-nil which may change the AuthzURLs slice in the call to m.deactivatePendingAuthz.
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 29, 2019
@dmitshur dmitshur added this to the Unreleased milestone Oct 29, 2019
@dmitshur dmitshur self-assigned this Oct 29, 2019
@gopherbot
Copy link

Change https://golang.org/cl/203919 mentions this issue: acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivatePendingAuthz

@dmitshur dmitshur mentioned this issue Oct 30, 2019
Closed
@golang golang locked and limited conversation to collaborators Oct 28, 2020
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@dmitshur
acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivat…
f155a23 
…ePendingAuthz

Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.

By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.

Fixes golang/go#35225
Updates golang/go#21081

Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@dmitshur
acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivat…
9f65086 
…ePendingAuthz

Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.

By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.

Fixes golang/go#35225
Updates golang/go#21081

Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@dmitshur
acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivat…
2482516 
…ePendingAuthz

Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.

By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.

Fixes golang/go#35225
Updates golang/go#21081

Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@dmitshur
acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivat…
f5cf380 
…ePendingAuthz

Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.

By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.

Fixes golang/go#35225
Updates golang/go#21081

Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@dmitshur
acme/autocert: always pass AuthzURLs from AuthorizeOrder to deactivat…
5ae9702 
…ePendingAuthz

Previously, the o.AuthzURLs slice was sometimes used from the call to
client.WaitOrder at the bottom of the for loop.

By that point, o may be nil if client.WaitOrder returned an error,
which would cause a nil pointer dereference panic inside the deferred
function call. If client.WaitOrder did not return an error, then the
call to deactivatePendingAuthz would use its AuthzURLs slice instead
of the one from client.AuthorizeOrder.

Fixes golang/go#35225
Updates golang/go#21081

Change-Id: I7db055ee1149871b6e5d34a8618526899c68f827
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/203919
Reviewed-by: Alex Vaghin <ddos@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
2 participants
@dmitshur @gopherbot