1424743 - scopeCompare doesn't compare scopes correctly

Status: RESOLVED FIXED

(Taskcluster :: Services, enhancement)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

[flagged as security-sensitive since it's not clear this wouldn't be exploitable]

Scopes

  - ab
  - abc*
  - abc
  - abc%d

should sort in that order.  However

> scopeCompare('abc*', 'abc%d');
1  # !!
> scopeCompare('abc*', 'abc');
-1
> scopeCompare('abc', 'abc%d');
-1

So the comparison doesn't even honor the transitive property.

And indeed, depending on the sort algorithm used (Rust's is different and uses fewer comparisons, I guess) this can produce incorrect results.

BTW I didn't modify this function (or even look at it) when refactoring auth.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

    testSortScopes({
      scopes: [
        'abc',
        'abc%d',
        'abc*',
        'abc*d',
        'ab',
        'abcd',
      ],  
      expected: [
        'ab',
        'abc*',
        'abc',
        'abc%d',                                                                                                                                                                                                                                                                            
        'abc*d',
        'abcd',
      ],  
    }); 

fails

      AssertionError [ERR_ASSERTION]: [ 'ab', 'abc*', 'abc', 'abc%d', 'abc*d', 'abcd' ] deepEqual [ 'ab', 'abc', 'abc%d', 'abc*', 'abc*d', 'abcd' ]
      + expected - actual

       [
         "ab"
      -  "abc*"
         "abc"
         "abc%d"
      +  "abc*"
         "abc*d"
         "abcd"
       ]

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

The issue seems to be limited to cases of <prefix>* vs <prefix><C><suffix> with ord(C) < ord('*').

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

On reflection, I think the only attack this would allow is "deactivating" someone's * scope, making it just a regular scope that matches only a literal *.  So not a huge security risk.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

https://github.com/taskcluster/taskcluster-lib-scopes/pull/19

Note the order for things with stars in them, e.g.

 ''
 '*'
 '**'
 '***'
 '****'

is asymmetrical:

 '*'
 ''
 '**'
 '***'
 '****'

I think that satisfies the trie construction requirements, right?

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

BTW, that's equivalent to

 '*'
 ''
 'X*'
 'XX*'
 'XXX*'

since those nonterminal *'s aren't kleen stars.

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Released as 1.6.5, and auth update to use it.