Quantcast

Hardening the roundup session id

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Hardening the roundup session id

John P. Rouillard
Hi all:

I have been trying to get a number of security improvements
working in my tracker:

  * Content Security Policy
  * Strict Transport Security
  * Public Key Pinning (report only mode for now)
  * X-XSS-Protection header set to block
  * X-Frame-Options set to sameorigin
  * Referrer-Policy
  * Turn off content type sniffing by browser (so files
      identified as plain/text that are html are treated
      as plain text and not interpreted as html because
      the browser recognizes them as html)
  * more CSRF protection

Except for some javascript injected by the core I have
things working with the CSP. I still need to add one more
keyword to the session cookie which again requires a change
to the core.

While I was working on this I came across this oddness.

When a user logs in they get assigned a random _sid. This is
sent to the browser in a cookie that can be used on
subsequent connections to identify/authenticate the user.

Since I have a requirement for a secure nonce I looked into
how that _sid was being generated.

I logged in as a user and after the login printed the new
_sid and a call to the function that generates the sid. Here
is the output:

 client.session_api._sid 'MTQ4OTM2MjE2Ny4yODAuNTczNzI0OTgwOTgx',
                new call 'MTQ4OTM2MjE2Ny4zODAuODQwMjA5OTA5NTc3'

note how similar the prefix is for both of these. The code
that generates the sid is:

     s = '%s%s'%(time.time(), random.random())
     s = binascii.b2a_base64(s).strip()

so I guess it's not surprising that the time.time component
looks identical. Does it make sense to change that to
something like:

     s = '%s%s'%(random.random(),time.time())
     s = hashlib.sha224(s.strip()).hexdigest()

(I swapped the order of the time and the random data so the
random data acts as a salt for the time in the sha224.)

Quips, comments?

Also is there anything built into the core that provides a
cryptographic hash tied to the client object? Basically for
every connection I want a new random piece of data. It's
going to be used as a nonce for "signing" <script> tags and
as such should not be guessable.

Thanks.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Hardening the roundup session id

Ralf Schlatterbeck-3
On Sun, Mar 12, 2017 at 08:10:30PM -0400, John P. Rouillard wrote:

> I logged in as a user and after the login printed the new
> _sid and a call to the function that generates the sid. Here
> is the output:
>
>  client.session_api._sid 'MTQ4OTM2MjE2Ny4yODAuNTczNzI0OTgwOTgx',
>                 new call 'MTQ4OTM2MjE2Ny4zODAuODQwMjA5OTA5NTc3'
>
> note how similar the prefix is for both of these. The code
> that generates the sid is:
>
>      s = '%s%s'%(time.time(), random.random())
>      s = binascii.b2a_base64(s).strip()
>
> so I guess it's not surprising that the time.time component
> looks identical. Does it make sense to change that to
> something like:
>
>      s = '%s%s'%(random.random(),time.time())
>      s = hashlib.sha224(s.strip()).hexdigest()
>
> (I swapped the order of the time and the random data so the
> random data acts as a salt for the time in the sha224.)
>
> Quips, comments?

Hmm, if we want to do this right random.random should not be used. AFAIK
it initializes the random number generator from time (), too. And random
is not cryptographically secure. So when concatenating time() and
random() here (in whatever order) you probably end up with something
that can be easily brute-forced just from guessing the time of the call.
This would work even if you know the time only with several hours of
accuracy :-)

> Also is there anything built into the core that provides a
> cryptographic hash tied to the client object?

No, that is one of the reasons we don't have good CSRF protection yet.

> Basically for every connection I want a new random piece of data. It's
> going to be used as a nonce for "signing" <script> tags and as such
> should not be guessable.

One possibility would be to derive such a thing from a hash (better
hmac) of the login-password (note: *not* the *hashed* login password
which should be asumed is known to the attacker) and the time of login,
e.g. generate a hmac (preferrably based on sha2 not sha1 ;-) with the
login-password as the secret and the login time as the input to the
hash. Keep this in a data structure similar to the current login
credentials (cookies etc).

With such a secret generating CSRF tokens etc. (again using a hmac with
this secret as the key) should be secure enough.

Note that I've implemented re-hashing of passwords with a more secure
hash on login some time ago. This re-hashes the user's password on login
with a more secure hash function depending on configuration (asuming the
old less secure hash hasn't leaked so far). So adding a CSRF-hash
outlined above in about the same position in the code (search for the
config-option 'migrate_passwords') should work.

I haven't got much time to look into this myself currently but would be
willing to look over an implementation.

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Hardening the roundup session id

John P. Rouillard
Hi Ralf:

In message <[hidden email]>,
Ralf Schlatterbeck writes:

>On Sun, Mar 12, 2017 at 08:10:30PM -0400, John P. Rouillard wrote:
>> I logged in as a user and after the login printed the new
>> _sid and a call to the function that generates the sid. Here
>> is the output:
>>
>>  client.session_api._sid 'MTQ4OTM2MjE2Ny4yODAuNTczNzI0OTgwOTgx',
>>                 new call 'MTQ4OTM2MjE2Ny4zODAuODQwMjA5OTA5NTc3'
>>
>> note how similar the prefix is for both of these. The code
>> that generates the sid is:
>>
>>      s = '%s%s'%(time.time(), random.random())
>>      s = binascii.b2a_base64(s).strip()
>>
>> so I guess it's not surprising that the time.time component
>> looks identical. Does it make sense to change that to
>> something like:
>>
>>      s = '%s%s'%(random.random(),time.time())
>>      s = hashlib.sha224(s.strip()).hexdigest()
>>
>> (I swapped the order of the time and the random data so the
>> random data acts as a salt for the time in the sha224.)
>>
>> Quips, comments?
>
>Hmm, if we want to do this right random.random should not be used. AFAIK
>it initializes the random number generator from time (), too.

There is a call to random.seed() in the init for client. IIUC that
should use /dev/random (or /dev/urandom).

   "None or no argument seeds from current time or from an operating
    system specific randomness source if available (see the os.urandom()
    function for details on availability)."

os.urandom should use /dev/urandom on unix or equiv on windows.

Also we have (since python 2.4):

    class random.SystemRandom([seed])

    Class that uses the os.urandom() function for generating random
    numbers from sources provided by the operating system. Not
    available on all systems. Does not rely on software state and
    sequences are not reproducible. Accordingly, the seed() and
    jumpahead() methods have no effect and are ignored. The getstate()
    and setstate() methods raise NotImplementedError if called.


    os.urandom(n)

    Return a string of n random bytes suitable for cryptographic use.

    This function returns random bytes from an OS-specific randomness
    source. The returned data should be unpredictable enough for
    cryptographic applications, though its exact quality depends on
    the OS implementation. On a UNIX-like system this will query
    /dev/urandom, and on Windows it will use CryptGenRandom(). If a
    randomness source is not found, NotImplementedError will be
    raised.

So should

  try:
     from random.SystemRandom import random
  except ....:
     from random import random

work to upgrade the current code?

I already added a client_nonce to the client class and initalize it in
__init__. I can change the new Client::_get_nonce() function to use a
better random source and we should be good there.

>is not cryptographically secure. So when concatenating time() and
>random() here (in whatever order) you probably end up with something
>that can be easily brute-forced just from guessing the time of the call.
>This would work even if you know the time only with several hours of
>accuracy :-)

Yeah some clocks are only good to the second so assuming the time call
for seeding random and the time call for the session id are
a second apart at most, calculate:

   nonce = random(time()) + time()
   nonce = random(time()-1) + time()
   nonce = random(time()+1) + time()

for 4 hours *3600s/h until you can log in is doable.

However if random.seed() initalizes using os.urandom the brute force
approach above goes out the window.

> Also is there anything built into the core that provides a
> cryptographic hash tied to the client object?
>
>No, that is one of the reasons we don't have good CSRF protection yet.

Ok, I wasn't sure if that ticket was still open.

>> Basically for every connection I want a new random piece of data. It's
>> going to be used as a nonce for "signing" <script> tags and as such
>> should not be guessable.
>
>One possibility would be to derive such a thing from a hash (better
>hmac) of the login-password (note: *not* the *hashed* login password
>which should be asumed is known to the attacker) and the time of login,
>e.g. generate a hmac (preferrably based on sha2 not sha1 ;-) with the
>login-password as the secret and the login time as the input to the
>hash. Keep this in a data structure similar to the current login
>credentials (cookies etc).
>
>With such a secret generating CSRF tokens etc. (again using a hmac with
>this secret as the key) should be secure enough.

Also I am not sure about incluing the password if os.random() is
sufficiently random.  Does including the password in the hash provide
a mechanism to reverse engineer the password if you get enough session
tokens or client_nonces?

Remember in all cases these tokens should be relatively short lived. A
few weeks probably for session at best and the client_nonce is
effectively single use. So making the cost to break the session token
longer than a month is probably sufficient. We could harden that in
the future by invalidating and reissuing the session token every
day/week etc.

Hashed passwords have much longer lives.

Given the above notes, are you suggesting:

  key = password + random(os.urandom()) + time()
  client_nonce = hmac.new(key, None, hashlib.sha256).hexdigest()

(note hmac defaults to using the hashlib.md5, so lets use sha256)

would

  key = password + random(os.urandom()) + time()
  client_nonce = hashlib.sha256(key).hexdigest()

also work?

>Note that I've implemented re-hashing of passwords with a more secure
>hash on login some time ago. This re-hashes the user's password on login
>with a more secure hash function depending on configuration (asuming the
>old less secure hash hasn't leaked so far). So adding a CSRF-hash
>outlined above in about the same position in the code (search for the
>config-option 'migrate_passwords') should work.

I remember that code. I'll look for it.

>I haven't got much time to look into this myself currently but would be
>willing to look over an implementation.

Sounds good. I'll try to get some diffs posted.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Hardening the roundup session id

Ralf Schlatterbeck-3
On Mon, Mar 13, 2017 at 08:46:53AM -0400, John P. Rouillard wrote:

> >Hmm, if we want to do this right random.random should not be used. AFAIK
> >it initializes the random number generator from time (), too.
>
> There is a call to random.seed() in the init for client. IIUC that
> should use /dev/random (or /dev/urandom).
>
>    "None or no argument seeds from current time or from an operating
>     system specific randomness source if available (see the os.urandom()
>     function for details on availability)."
>
> os.urandom should use /dev/urandom on unix or equiv on windows.
>
> Also we have (since python 2.4):
>
>     class random.SystemRandom([seed])
>
>     Class that uses the os.urandom() function for generating random
>     numbers from sources provided by the operating system. Not
>     available on all systems. Does not rely on software state and
>     sequences are not reproducible. Accordingly, the seed() and
>     jumpahead() methods have no effect and are ignored. The getstate()
>     and setstate() methods raise NotImplementedError if called.
>
>
>     os.urandom(n)
>
>     Return a string of n random bytes suitable for cryptographic use.
>
>     This function returns random bytes from an OS-specific randomness
>     source. The returned data should be unpredictable enough for
>     cryptographic applications, though its exact quality depends on
>     the OS implementation. On a UNIX-like system this will query
>     /dev/urandom, and on Windows it will use CryptGenRandom(). If a
>     randomness source is not found, NotImplementedError will be
>     raised.

Cool. I didn't know that.

> So should
>
>   try:
>      from random.SystemRandom import random
>   except ....:
>      from random import random
>
> work to upgrade the current code?

Yes, but the fallback will probably be a lot less secure.

> I already added a client_nonce to the client class and initalize it in
> __init__. I can change the new Client::_get_nonce() function to use a
> better random source and we should be good there.

Cool, yes that should work.

[...]
> However if random.seed() initalizes using os.urandom the brute force
> approach above goes out the window.
>
> > Also is there anything built into the core that provides a
> > cryptographic hash tied to the client object?
> >
> >No, that is one of the reasons we don't have good CSRF protection yet.
>
> Ok, I wasn't sure if that ticket was still open.

I'm also not sure but don't remember seeing it closed.

> Also I am not sure about incluing the password if os.random() is
> sufficiently random.  Does including the password in the hash provide
> a mechanism to reverse engineer the password if you get enough session
> tokens or client_nonces?
No, if using a hmac hash that should be secure against reversing the
password.

> Remember in all cases these tokens should be relatively short lived. A
> few weeks probably for session at best and the client_nonce is
> effectively single use. So making the cost to break the session token
> longer than a month is probably sufficient. We could harden that in
> the future by invalidating and reissuing the session token every
> day/week etc.
>
> Hashed passwords have much longer lives.
>
> Given the above notes, are you suggesting:
>
>   key = password + random(os.urandom()) + time()
>   client_nonce = hmac.new(key, None, hashlib.sha256).hexdigest()
>
> (note hmac defaults to using the hashlib.md5, so lets use sha256)

Yes something along these lines. But if the os.random uses a secure
source of entropy I'm fine not using the password at all.

>
> would
>
>   key = password + random(os.urandom()) + time()
>   client_nonce = hashlib.sha256(key).hexdigest()
>
> also work?

Yes, but the hmac seems to be accepted as being a little more secure
than a simple hash (against brute-forcing the password).

So given your proposals for using a secure randomness source I'd not
use the user password for deriving the nonce.

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Hardening the roundup session id

Joseph S. Myers
In reply to this post by John P. Rouillard
On Mon, 13 Mar 2017, John P. Rouillard wrote:

> > Also is there anything built into the core that provides a
> > cryptographic hash tied to the client object?
> >
> >No, that is one of the reasons we don't have good CSRF protection yet.
>
> Ok, I wasn't sure if that ticket was still open.

http://issues.roundup-tracker.org/issue2550690 is the issue I filed for
CSRF - I don't think there have been any improvements in that regard since
I filed it.

I concluded at that time that the seeding of the random number generator
for each request, together with the limited amount of use of random
numbers after it is seeded (so not enough are used for it to be relevant
that it's a non-cryptographically-secure generator seeded with a secure
seed), meant that the then-current random number uses were secure,
although it would be better to use os.urandom() or equivalent directly
whenever secure random numbers are required (session ids,
generatePassword, ...), rather than relying on only a small amount of
random data being generated after seeding.

--
Joseph S. Myers
[hidden email]

------------------------------------------------------------------------------
Announcing the Oxford Dictionaries API! The API offers world-renowned
dictionary content that is easy and intuitive to access. Sign up for an
account today to start using our lexical data to power your apps and
projects. Get started today and enter our developer competition.
http://sdm.link/oxford
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

CSRF implementation take 1

John P. Rouillard

Hi all:

I have attached a first pass at anti CSRF measures.

Basic idea:

  in Client::inner_main change the flow and put in handle_csrf() If
     it returns True (yeah I know not pythonic) it allows the
     handle_action code to run to handle @action etc. If it returns
     False it redisplays the source page with errors added by
     add_error_message.

  Add code to generate csrf tokens that are stored in the one time key
     store that expire after two weeks. Make submit buttons
     automatically generate the hidden input and call the csrf
     generator. Also provide a csrf generator in templating utils
     so people can generate their own @csrf tokens.

 In roundup_server add some headers useful for detecting CSRF issues
   * Origin
   * Referer
   * X-Forwarded-Host

Use these headers as part of the handle_csrf code.

Handle_Csrf returns true if the method is not post.  It then compares
the three headers to the base url and reports an issue if they don't
compare well.  It then requires a CSRF token and looks it up in the
one time key store. It gets the value of the sid and uid (the 3 in
user3) and compares them to the session id and user id of the current
client session. If they are not all equal it returns false and sets
error_message. If they are all true, it destroys the csrf token and
returns true.


Known issues: I have three copies of the code to generate csrf nonce.

I couldn't figure out how to have one copy of the code and include it
as a method in all three classes. Maybe generate a new csrf class and
define the code there and derive the other three classes from it??

Patch attached. I am off to bed.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel

csrf_patch1.patch (11K) Download Attachment
Loading...