Browse code
plugin: Fix off-by-one in privilege validation
Fix an off-by-one error in isEqual() where the comparison loop started
at index 1 instead of 0, causing the first privilege (after sorting
alphabetically by name) to never be validated.
This allowed a malicious plugin to request different values for
whichever privilege sorts first — most notably "allow-all-devices",
which grants unrestricted rwm access to all host devices.
The bug also meant that plugins requesting exactly one privilege had
zero iterations of the comparison loop, bypassing validation entirely.
Also fix an existing test case ("diff-order-but-same-value") that only
passed due to the off-by-one bug, and add test cases for single-element
and first-sorted-element mismatches.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Paweł Gronowski authored on 2026/03/20 03:18:23Showing 2 changed files
| ... | ... |
@@ -1,13 +1,13 @@ |
| 1 | 1 |
package plugin |
| 2 | 2 |
|
| 3 | 3 |
import ( |
| 4 |
+ "cmp" |
|
| 4 | 5 |
"context" |
| 5 | 6 |
"encoding/json" |
| 6 | 7 |
"io" |
| 7 | 8 |
"os" |
| 8 | 9 |
"path/filepath" |
| 9 |
- "reflect" |
|
| 10 |
- "sort" |
|
| 10 |
+ "slices" |
|
| 11 | 11 |
"strings" |
| 12 | 12 |
"sync" |
| 13 | 13 |
"syscall" |
| ... | ... |
@@ -337,34 +337,42 @@ func makeLoggerStreams(id string) (stdout, stderr io.WriteCloser) {
|
| 337 | 337 |
} |
| 338 | 338 |
|
| 339 | 339 |
func validatePrivileges(requiredPrivileges, privileges plugin.Privileges) error {
|
| 340 |
- if !isEqual(requiredPrivileges, privileges, isEqualPrivilege) {
|
|
| 340 |
+ if len(requiredPrivileges) != len(privileges) {
|
|
| 341 | 341 |
return errors.New("incorrect privileges")
|
| 342 | 342 |
} |
| 343 | 343 |
|
| 344 |
- return nil |
|
| 345 |
-} |
|
| 346 |
- |
|
| 347 |
-func isEqual(arrOne, arrOther plugin.Privileges, compare func(x, y plugin.Privilege) bool) bool {
|
|
| 348 |
- if len(arrOne) != len(arrOther) {
|
|
| 349 |
- return false |
|
| 350 |
- } |
|
| 351 |
- |
|
| 352 |
- sort.Sort(arrOne) |
|
| 353 |
- sort.Sort(arrOther) |
|
| 344 |
+ a := normalizePrivileges(requiredPrivileges) |
|
| 345 |
+ b := normalizePrivileges(privileges) |
|
| 354 | 346 |
|
| 355 |
- for i := 1; i < arrOne.Len(); i++ {
|
|
| 356 |
- if !compare(arrOne[i], arrOther[i]) {
|
|
| 357 |
- return false |
|
| 347 |
+ for i := range a {
|
|
| 348 |
+ if a[i].Name != b[i].Name {
|
|
| 349 |
+ return errors.New("incorrect privileges")
|
|
| 350 |
+ } |
|
| 351 |
+ if !slices.Equal(a[i].Value, b[i].Value) {
|
|
| 352 |
+ return errors.New("incorrect privileges")
|
|
| 358 | 353 |
} |
| 359 | 354 |
} |
| 360 | 355 |
|
| 361 |
- return true |
|
| 356 |
+ return nil |
|
| 362 | 357 |
} |
| 363 | 358 |
|
| 364 |
-func isEqualPrivilege(a, b plugin.Privilege) bool {
|
|
| 365 |
- if a.Name != b.Name {
|
|
| 366 |
- return false |
|
| 359 |
+// normalizePrivileges returns a normalized copy of privileges with privilege names |
|
| 360 |
+// and each privilege's values sorted for order-insensitive comparison. |
|
| 361 |
+// The input is not mutated. |
|
| 362 |
+func normalizePrivileges(privileges plugin.Privileges) plugin.Privileges {
|
|
| 363 |
+ normalized := make(plugin.Privileges, len(privileges)) |
|
| 364 |
+ for i, privilege := range privileges {
|
|
| 365 |
+ normalized[i] = plugin.Privilege{
|
|
| 366 |
+ Name: privilege.Name, |
|
| 367 |
+ Description: privilege.Description, |
|
| 368 |
+ Value: slices.Clone(privilege.Value), |
|
| 369 |
+ } |
|
| 370 |
+ slices.Sort(normalized[i].Value) |
|
| 367 | 371 |
} |
| 368 | 372 |
|
| 369 |
- return reflect.DeepEqual(a.Value, b.Value) |
|
| 373 |
+ slices.SortFunc(normalized, func(a, b plugin.Privilege) int {
|
|
| 374 |
+ return cmp.Compare(a.Name, b.Name) |
|
| 375 |
+ }) |
|
| 376 |
+ |
|
| 377 |
+ return normalized |
|
| 370 | 378 |
} |
| ... | ... |
@@ -44,12 +44,48 @@ func TestValidatePrivileges(t *testing.T) {
|
| 44 | 44 |
}, |
| 45 | 45 |
result: true, |
| 46 | 46 |
}, |
| 47 |
+ "single-element-same": {
|
|
| 48 |
+ requiredPrivileges: []plugin.Privilege{
|
|
| 49 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"true"}},
|
|
| 50 |
+ }, |
|
| 51 |
+ privileges: []plugin.Privilege{
|
|
| 52 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"true"}},
|
|
| 53 |
+ }, |
|
| 54 |
+ result: true, |
|
| 55 |
+ }, |
|
| 56 |
+ "single-element-diff-value": {
|
|
| 57 |
+ requiredPrivileges: []plugin.Privilege{
|
|
| 58 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"false"}},
|
|
| 59 |
+ }, |
|
| 60 |
+ privileges: []plugin.Privilege{
|
|
| 61 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"true"}},
|
|
| 62 |
+ }, |
|
| 63 |
+ result: false, |
|
| 64 |
+ }, |
|
| 65 |
+ "first-sorted-element-diff-value": {
|
|
| 66 |
+ requiredPrivileges: []plugin.Privilege{
|
|
| 67 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"false"}},
|
|
| 68 |
+ {Name: "network", Description: "Description", Value: []string{"host"}},
|
|
| 69 |
+ }, |
|
| 70 |
+ privileges: []plugin.Privilege{
|
|
| 71 |
+ {Name: "allow-all-devices", Description: "Description", Value: []string{"true"}},
|
|
| 72 |
+ {Name: "network", Description: "Description", Value: []string{"host"}},
|
|
| 73 |
+ }, |
|
| 74 |
+ result: false, |
|
| 75 |
+ }, |
|
| 76 |
+ "empty-privileges": {
|
|
| 77 |
+ requiredPrivileges: []plugin.Privilege{},
|
|
| 78 |
+ privileges: []plugin.Privilege{},
|
|
| 79 |
+ result: true, |
|
| 80 |
+ }, |
|
| 47 | 81 |
} |
| 48 | 82 |
|
| 49 | 83 |
for key, data := range testData {
|
| 50 |
- err := validatePrivileges(data.requiredPrivileges, data.privileges) |
|
| 51 |
- if (err == nil) != data.result {
|
|
| 52 |
- t.Fatalf("Test item %s expected result to be %t, got %t", key, data.result, (err == nil))
|
|
| 53 |
- } |
|
| 84 |
+ t.Run(key, func(t *testing.T) {
|
|
| 85 |
+ err := validatePrivileges(data.requiredPrivileges, data.privileges) |
|
| 86 |
+ if (err == nil) != data.result {
|
|
| 87 |
+ t.Fatalf("expected result to be %t, got %t", data.result, (err == nil))
|
|
| 88 |
+ } |
|
| 89 |
+ }) |
|
| 54 | 90 |
} |
| 55 | 91 |
} |