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

fix race condition in string generator helper #19875

Merged
merged 2 commits into from
Mar 31, 2023
Merged

fix race condition in string generator helper #19875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa1ce70
fix race condition in string generator helper
fairclothjm Mar 30, 2023
33b6df3
add changelog
fairclothjm Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/19875.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
helper/random: Fix race condition in string generator helper
```
11 changes: 9 additions & 2 deletions helper/random/string_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"io"
"math"
"sort"
"sync"
"time"
"unicode"

Expand Down Expand Up @@ -79,7 +80,8 @@ type StringGenerator struct {
Rules serializableRules `mapstructure:"-" json:"rule"` // This is "rule" in JSON so it matches the HCL property type

// CharsetRule to choose runes from. This is computed from the rules, not directly configurable
charset runes
charset runes
charsetLock sync.RWMutex
}

// Generate a random string from the charset and adhering to the provided rules.
Expand Down Expand Up @@ -119,7 +121,10 @@ func (g *StringGenerator) generate(rng io.Reader) (str string, err error) {
// If performance improvements need to be made, this can be changed to read a batch of
// potential strings at once rather than one at a time. This will significantly
// improve performance, but at the cost of added complexity.
candidate, err := randomRunes(rng, g.charset, g.Length)
g.charsetLock.RLock()
charset := g.charset
g.charsetLock.RUnlock()
Comment on lines +124 to +126
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why locking is needed for setting a variable like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The lock isn't necessary for the write to the local charset variable, but the read of the g.charset value.

In validateConfig: 
if len(g.charset) == 0 {
    g.charset = getChars(g.Rules)
}

so if there's parallel access to both validateConfig and generate, we will race reading this variable. Since this assignment isn't an atomic write (its a []rune slice, which is a pointer type), you might read a bad pointer (like, half of the pointer's value could be copied over when you read, so you'd point to a garbage memory location) and thus fail.

In particular, I think when you first initialize the group and set it up to do a password rotation, if you have a lot of in-flight requests, you'll see multiple try and write this charset via validating the config I believe, and some which will be past it trying to do this generate read already.

Or maybe it is parallel modification + use of the policy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cipherboy! That is a great summary.

@robmonte I am happy to talk more about this if you have more questions. Let me know!

candidate, err := randomRunes(rng, charset, g.Length)
if err != nil {
return "", fmt.Errorf("unable to generate random characters: %w", err)
}
Expand Down Expand Up @@ -232,6 +237,8 @@ func (g *StringGenerator) validateConfig() (err error) {
merr = multierror.Append(merr, fmt.Errorf("specified rules require at least %d characters but %d is specified", minLen, g.Length))
}

g.charsetLock.Lock()
defer g.charsetLock.Unlock()
// Ensure we have a charset & all characters are printable
if len(g.charset) == 0 {
// Yes this is mutating the generator but this is done so we don't have to compute this on every generation
Expand Down