Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204398
---



PASS: Mesos patch 67357 was successfully built and tested.

Reviews applied: `['67357']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357

- Mesos Reviewbot Windows


On June 6, 2018, 1:45 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 1:45 p.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/4/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rojas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/
---

(Updated June 6, 2018, 3:34 p.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

A vulnerability in our JWT implementation allows an unauthenticated
remote attacker to execute to execute timing attacks [1].

This patch removes the vulnerability by adding a constant time
comparison of hashes, where the whole message is visited during
the comparison instead of returning at the first failure.

[1] https://codahale.com/a-lesson-in-timing-attacks/


Diffs (updated)
-

  3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 


Diff: https://reviews.apache.org/r/67357/diff/4/

Changes: https://reviews.apache.org/r/67357/diff/3-4/


Testing
---

```sh
make check
```


Thanks,

Alexander Rojas

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rukletsov 


> On June 6, 2018, 11:58 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 213 (patched)
> > 
> >
> > To avoid conversions and integral promotions, does it make sense to say 
> > here
> > ```
> > string::value_type valid = 0;
> > ```
> > ?
> 
> Benno Evers wrote:
> I would recommend against that, as it would turn `valid` back into a 
> signed type and even obfuscate that fact. As I said before, it's good 
> practice to only do bitwise operation on unsigned types.
> 
> For example (admittedly, its a bit far-fetched, but bear with me), 
> imagine a system using ones complement. Two different characters could xor to 
> produce a negative zero, and implementations are permitted to convert that to 
> positive zero, producing a false positive.

Discussed this offline with Benno and Alexander. If `char ^ char` produces `0`, 
during integer promotion it is padded with `0`s and `OR`d with unsigned 
correctly. If `char ^ char` does not produce `0`, it can be padded with either 
`1`s or `0`s, but we don't really care since the result of `OR` with unsigned 
will be non-zero. The code in the current version seems to be correct.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204378
---


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rojas 


> On June 6, 2018, 12:26 p.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['67357']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357
> > 
> > Relevant logs:
> > 
> > - 
> > [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stdout.log):
> > 
> > ```
> > [ RUN  ] SlaveTest.ResourceVersions
> > [   OK ] SlaveTest.ResourceVersions (214 ms)
> > [ RUN  ] SlaveTest.ReconfigurationPolicy
> > [   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
> > [ RUN  ] SlaveTest.ResourceProviderReconciliation
> > [   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
> > [ RUN  ] SlaveTest.RunTaskResourceVersions
> > [   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
> > [--] 83 tests from SlaveTest (70519 ms total)
> > 
> > [--] 3 tests from SlaveStateTest
> > [ RUN  ] SlaveStateTest.CheckpointString
> > [   OK ] SlaveStateTest.CheckpointString (4 ms)
> > [ RUN  ] SlaveStateTest.CheckpointProtobufMessage
> > [   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
> > [ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
> > [   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
> > [--] 3 tests from SlaveStateTest (25 ms total)
> > 
> > [--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
> > mesos::internal::slave::MesosContainerizer
> > [ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
> > [   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
> > [ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
> > [   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
> > [ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
> > ```
> > 
> > - 
> > [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stderr.log):
> > 
> > ```
> > @   7FF71117D888  
> > std::invoke<,process::Future
> >  >,process::ProcessBase *>
> > @   7FF71119257B  
> > lambda::internal::Partial<,process::Future
> >  >,std::_Ph<1> 
> > >::invoke_expand<,std::tuple
> >  >,std::_Ph<1> >,st
> > @   7FF7110C08BA  ) > @   7FF7110F058C  
> > std::_Invoker_functor::_Call,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF711183EBC  
> > std::invoke,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF7110C9F21  
> > ),process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *
> > @   7FF711236416  process::ProcessBase 
> > *)>::CallableFn,process::Future
> >  >,std::_Ph<1> > >::operator(
> > @   7FF712C1A25D  process::ProcessBase *)>::operator(
> > @   7FF712ACB2F9  process::ProcessBase::consume
> > @   7FF712C738CA  process::DispatchEvent::consume
> > @   7FF70ECE7B07  process::ProcessBase::serve
> > @   7FF712AD93B0  process::ProcessManager::resume
> > @   7FF712C07371   ?? 
> > @   7FF712B2B130  
> > std::_Invoker_functor::_Call< >
> > @   7FF712B8B8E0  
> > std::invoke< >
> > @   7FF712B4076C  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Execute<0>
> > @   7FF712C5A60A  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Run
> > @   7FF712C45E78  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Go
> > @   7FF712C2C3CD  std::_Pad::_Call_func
> > @   7FFF9BE53428  _register_onexit_function
> > @   7FFF9BE53071  _register_onexit_function
> > @   7FFFB6391FE4  BaseThreadInitThunk
> > @   7FFFB69FF061  RtlUserThreadStart
> > ll containerizers
> > I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
> > I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
> > executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
> > I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration 
> > message from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451-
> > I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
> > within 75secs
> > F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == 
> > DISCONNECTED || state == RUNNING || state == TERMINATING RECOVERING
> > ```
> 
> Alexander Rukletsov wrote:
> WTF is this... Can you please check the JIRA and file an issue if it is a 
> new one?

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Benno Evers 


> On May 31, 2018, 4:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 205 (patched)
> > 
> >
> > To be more precise: "which compares either none of the content or the 
> > whole content of the strings".
> > 
> > (but actually I'm not even sure a comment is needed at all, the 
> > function name kinda speaks for itself)
> 
> Alexander Rojas wrote:
> I agree but the shepherd requested it ;)

Hehe, can't argue with that :D


> On May 31, 2018, 4:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 215 (patched)
> > 
> >
> > I think its safer to use unsigned types for bit manipulation, otherwise 
> > it's really easy to accidentally trigger undefined behaviour. (i.e. even 
> > here I'm not sure its safe off the top of my head, since the xor could flip 
> > the sign bit of the chars)

Hm, so `left[i] ^ right[i]` is still an xor of signed chars, but since its 
actually safe the way its used here, I guess its up to personal preference if 
you want to add an explicit cast.


- Benno


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204122
---


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Benno Evers 


> On June 6, 2018, 11:58 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 213 (patched)
> > 
> >
> > To avoid conversions and integral promotions, does it make sense to say 
> > here
> > ```
> > string::value_type valid = 0;
> > ```
> > ?

I would recommend against that, as it would turn `valid` back into a signed 
type and even obfuscate that fact. As I said before, it's good practice to only 
do bitwise operation on unsigned types.

For example (admittedly, its a bit far-fetched, but bear with me), imagine a 
system using ones complement. Two different characters could xor to produce a 
negative zero, and implementations are permitted to convert that to positive 
zero, producing a false positive.


- Benno


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204378
---


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/3/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rukletsov 


> On June 6, 2018, 10:26 a.m., Mesos Reviewbot Windows wrote:
> > FAIL: Some of the unit tests failed. Please check the relevant logs.
> > 
> > Reviews applied: `['67357']`
> > 
> > Failed command: `Start-MesosCITesting`
> > 
> > All the build artifacts available at: 
> > http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357
> > 
> > Relevant logs:
> > 
> > - 
> > [mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stdout.log):
> > 
> > ```
> > [ RUN  ] SlaveTest.ResourceVersions
> > [   OK ] SlaveTest.ResourceVersions (214 ms)
> > [ RUN  ] SlaveTest.ReconfigurationPolicy
> > [   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
> > [ RUN  ] SlaveTest.ResourceProviderReconciliation
> > [   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
> > [ RUN  ] SlaveTest.RunTaskResourceVersions
> > [   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
> > [--] 83 tests from SlaveTest (70519 ms total)
> > 
> > [--] 3 tests from SlaveStateTest
> > [ RUN  ] SlaveStateTest.CheckpointString
> > [   OK ] SlaveStateTest.CheckpointString (4 ms)
> > [ RUN  ] SlaveStateTest.CheckpointProtobufMessage
> > [   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
> > [ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
> > [   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
> > [--] 3 tests from SlaveStateTest (25 ms total)
> > 
> > [--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
> > mesos::internal::slave::MesosContainerizer
> > [ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
> > [   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
> > [ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
> > [   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
> > [ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
> > [   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
> > [ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
> > ```
> > 
> > - 
> > [mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stderr.log):
> > 
> > ```
> > @   7FF71117D888  
> > std::invoke<,process::Future
> >  >,process::ProcessBase *>
> > @   7FF71119257B  
> > lambda::internal::Partial<,process::Future
> >  >,std::_Ph<1> 
> > >::invoke_expand<,std::tuple
> >  >,std::_Ph<1> >,st
> > @   7FF7110C08BA  ) > @   7FF7110F058C  
> > std::_Invoker_functor::_Call,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF711183EBC  
> > std::invoke,process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *>
> > @   7FF7110C9F21  
> > ),process::Future
> >  >,std::_Ph<1> >,process::ProcessBase *
> > @   7FF711236416  process::ProcessBase 
> > *)>::CallableFn,process::Future
> >  >,std::_Ph<1> > >::operator(
> > @   7FF712C1A25D  process::ProcessBase *)>::operator(
> > @   7FF712ACB2F9  process::ProcessBase::consume
> > @   7FF712C738CA  process::DispatchEvent::consume
> > @   7FF70ECE7B07  process::ProcessBase::serve
> > @   7FF712AD93B0  process::ProcessManager::resume
> > @   7FF712C07371   ?? 
> > @   7FF712B2B130  
> > std::_Invoker_functor::_Call< >
> > @   7FF712B8B8E0  
> > std::invoke< >
> > @   7FF712B4076C  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Execute<0>
> > @   7FF712C5A60A  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Run
> > @   7FF712C45E78  
> > std::_LaunchPad
> >  >,std::default_delete 
> > > > > >::_Go
> > @   7FF712C2C3CD  std::_Pad::_Call_func
> > @   7FFF9BE53428  _register_onexit_function
> > @   7FFF9BE53071  _register_onexit_function
> > @   7FFFB6391FE4  BaseThreadInitThunk
> > @   7FFFB69FF061  RtlUserThreadStart
> > ll containerizers
> > I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
> > I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
> > executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
> > I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration 
> > message from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
> > 62cf792a-dc69-4e3c-b54f-d83f98fb9451-
> > I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
> > within 75secs
> > F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == 
> > DISCONNECTED || state == RUNNING || state == TERMINATING RECOVERING
> > ```

WTF is this... Can you please check the JIRA and file an issue if it is a new 
one?


- Alexander

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204374
---



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['67357']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stdout.log):

```
[ RUN  ] SlaveTest.ResourceVersions
[   OK ] SlaveTest.ResourceVersions (214 ms)
[ RUN  ] SlaveTest.ReconfigurationPolicy
[   OK ] SlaveTest.ReconfigurationPolicy (317 ms)
[ RUN  ] SlaveTest.ResourceProviderReconciliation
[   OK ] SlaveTest.ResourceProviderReconciliation (358 ms)
[ RUN  ] SlaveTest.RunTaskResourceVersions
[   OK ] SlaveTest.RunTaskResourceVersions (306 ms)
[--] 83 tests from SlaveTest (70519 ms total)

[--] 3 tests from SlaveStateTest
[ RUN  ] SlaveStateTest.CheckpointString
[   OK ] SlaveStateTest.CheckpointString (4 ms)
[ RUN  ] SlaveStateTest.CheckpointProtobufMessage
[   OK ] SlaveStateTest.CheckpointProtobufMessage (9 ms)
[ RUN  ] SlaveStateTest.CheckpointRepeatedProtobufMessages
[   OK ] SlaveStateTest.CheckpointRepeatedProtobufMessages (10 ms)
[--] 3 tests from SlaveStateTest (25 ms total)

[--] 30 tests from SlaveRecoveryTest/0, where TypeParam = class 
mesos::internal::slave::MesosContainerizer
[ RUN  ] SlaveRecoveryTest/0.RecoverSlaveState
[   OK ] SlaveRecoveryTest/0.RecoverSlaveState (1573 ms)
[ RUN  ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager
[   OK ] SlaveRecoveryTest/0.RecoverTaskStatusUpdateManager (3328 ms)
[ RUN  ] SlaveRecoveryTest/0.ReconnectExecutor
[   OK ] SlaveRecoveryTest/0.ReconnectExecutor (3707 ms)
[ RUN  ] SlaveRecoveryTest/0.ReconnectExecutorRetry
[   OK ] SlaveRecoveryTest/0.ReconnectExecutorRetry (1205 ms)
[ RUN  ] SlaveRecoveryTest/0.PingTimeoutDuringRecovery
```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357/logs/mesos-tests-stderr.log):

```
@   7FF71117D888  
std::invoke<,process::Future
 >,process::ProcessBase *>
@   7FF71119257B  
lambda::internal::Partial<,process::Future
 >,std::_Ph<1> 
>::invoke_expand<,std::tuple
 >,std::_Ph<1> >,st
@   7FF7110C08BA  ),process::Future
 >,std::_Ph<1> >,process::ProcessBase *>
@   7FF711183EBC  
std::invoke,process::Future
 >,std::_Ph<1> >,process::ProcessBase *>
@   7FF7110C9F21  
),process::Future
 >,std::_Ph<1> >,process::ProcessBase *
@   7FF711236416  process::ProcessBase 
*)>::CallableFn,process::Future
 >,std::_Ph<1> > >::operator(
@   7FF712C1A25D  process::ProcessBase *)>::operator(
@   7FF712ACB2F9  process::ProcessBase::consume
@   7FF712C738CA  process::DispatchEvent::consume
@   7FF70ECE7B07  process::ProcessBase::serve
@   7FF712AD93B0  process::ProcessManager::resume
@   7FF712C07371   ?? 
@   7FF712B2B130  
std::_Invoker_functor::_Call< >
@   7FF712B8B8E0  std::invoke< 
>
@   7FF712B4076C  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Execute<0>
@   7FF712C5A60A  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Run
@   7FF712C45E78  
std::_LaunchPad
 >,std::default_delete > > 
> >::_Go
@   7FF712C2C3CD  std::_Pad::_Call_func
@   7FFF9BE53428  _register_onexit_function
@   7FFF9BE53071  _register_onexit_function
@   7FFFB6391FE4  BaseThreadInitThunk
@   7FFFB69FF061  RtlUserThreadStart
ll containerizers
I0606 10:25:26.680230 18356 slave.cpp:7158] Recovering executors
I0606 10:25:26.680230 18356 slave.cpp:7182] Sending reconnect request to 
executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
62cf792a-dc69-4e3c-b54f-d83f98fb9451- at executor(1)@192.10.1.5:55652
I0606 10:25:26.688225 22560 slave.cpp:4984] Received re-registration message 
from executor '3f11d255-bb7b-4e99-967b-055fef95b595' of framework 
62cf792a-dc69-4e3c-b54f-d83f98fb9451-
I0606 10:25:26.691216 22888 slave.cpp:5901] No pings from master received 
within 75secs
F0606 10:25:26.692219 22888 slave.cpp:1249] Check failed: state == DISCONNECTED 
|| state == RUNNING || state == TERMINATING RECOVERING
```

- Mesos Reviewbot Windows


On June 6, 2018, 8:54 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated June 6, 2018, 8:54 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rojas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/
---

(Updated June 6, 2018, 10:31 a.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

A vulnerability in our JWT implementation allows an unauthenticated
remote attacker to execute to execute timing attacks [1].

This patch removes the vulnerability by adding a constant time
comparison of hashes, where the whole message is visited during
the comparison instead of returning at the first failure.

[1] https://codahale.com/a-lesson-in-timing-attacks/


Diffs (updated)
-

  3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 


Diff: https://reviews.apache.org/r/67357/diff/3/

Changes: https://reviews.apache.org/r/67357/diff/2-3/


Testing
---

```sh
make check
```


Thanks,

Alexander Rojas

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-06-06 Thread Alexander Rojas 


> On May 31, 2018, 6:56 p.m., Benno Evers wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Lines 205 (patched)
> > 
> >
> > To be more precise: "which compares either none of the content or the 
> > whole content of the strings".
> > 
> > (but actually I'm not even sure a comment is needed at all, the 
> > function name kinda speaks for itself)

I agree but the shepherd requested it ;)


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204122
---


On May 31, 2018, 11:02 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated May 31, 2018, 11:02 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-05-31 Thread Alexander Rukletsov 


> On May 30, 2018, 1:37 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/jwt.cpp
> > Line 253 (original), 253-261 (patched)
> > 
> >
> > I suggest to exatract it into a funciton, say `compareConstantTime`.
> 
> Alexander Rojas wrote:
> `compareConstantTime` is not a good name, since in C++ _compare_ 
> functions always returns an integer which, if less than zero means left is 
> greater than right, greater than zero means right is greater than left and 0 
> means equality, see 
> [std::comapre](http://www.cplusplus.com/reference/string/string/compare/) and 
> [memcmp](http://www.cplusplus.com/reference/cstring/memcmp/) for examples.
> 
> I also don't think it makes a lot of sense to move this little snippet to 
> a new fuction given that is only used here. But I will do it anyway.

Good point about the name, Alexander.

Regarding a separate function, I think it is much easier to spot this snippet 
if someone needs something similar in the future. Plus small doing-one-thing 
with no side effects routines simplify reading the code significantly.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204048
---


On May 31, 2018, 9:02 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated May 31, 2018, 9:02 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-05-31 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review204109
---



PASS: Mesos patch 67357 was successfully built and tested.

Reviews applied: `['67357']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357

- Mesos Reviewbot Windows


On May 31, 2018, 11:02 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated May 31, 2018, 11:02 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing attacks [1].
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> [1] https://codahale.com/a-lesson-in-timing-attacks/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-05-31 Thread Alexander Rojas 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/
---

(Updated May 31, 2018, 11:02 a.m.)


Review request for Alexander Rukletsov.


Repository: mesos


Description (updated)
---

A vulnerability in our JWT implementation allows an unauthenticated
remote attacker to execute to execute timing attacks [1].

This patch removes the vulnerability by adding a constant time
comparison of hashes, where the whole message is visited during
the comparison instead of returning at the first failure.

[1] https://codahale.com/a-lesson-in-timing-attacks/


Diffs (updated)
-

  3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 


Diff: https://reviews.apache.org/r/67357/diff/2/

Changes: https://reviews.apache.org/r/67357/diff/1-2/


Testing
---

```sh
make check
```


Thanks,

Alexander Rojas

Re: Review Request 67357: Added constant time comparison of JWT signatures.

2018-05-29 Thread Mesos Reviewbot Windows 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67357/#review203996
---



PASS: Mesos patch 67357 was successfully built and tested.

Reviews applied: `['67357']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67357

- Mesos Reviewbot Windows


On May 29, 2018, 7:22 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67357/
> ---
> 
> (Updated May 29, 2018, 7:22 a.m.)
> 
> 
> Review request for Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A vulnerability in our JWT implementation allows an unauthenticated
> remote attacker to execute to execute timing 
> [attacks](https://codahale.com/a-lesson-in-timing-attacks/).
> 
> This patch removes the vulnerability by adding a constant time
> comparison of hashes, where the whole message is visited during
> the comparison instead of returning at the first failure.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/jwt.cpp 4477ddd17dede2b924a47e33942b39244f10316f 
> 
> 
> Diff: https://reviews.apache.org/r/67357/diff/1/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>