Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Android secure connection crash. #89

Open
kochol opened this issue Mar 4, 2016 · 7 comments
Open

Android secure connection crash. #89

kochol opened this issue Mar 4, 2016 · 7 comments

Comments

@kochol
Copy link

kochol commented Mar 4, 2016

Hi I use raknet secure connections in my game but it crash in android here is logcat log.

F/libc (18590): Fatal signal 7 (SIGBUS), code 1, fault addr 0x96d62c87 in tid 18665 (Thread-5992)
W/libc (18590): Security Level: (1), Debug inforamtion is controlled by the DUMPABLE flag.
I/DEBUG ( 446): *** *** *** *** *** *** *** *** *** *** *** *** *** *** * ***
I/DEBUG ( 446): Build fingerprint: 'htc/htc_asia_tw/htc_eyetuhl:5.0.2/LRX22G/653658.2:user/release-keys'
I/DEBUG ( 446): Revision: '0'
I/DEBUG ( 446): ABI: 'arm'
I/DEBUG ( 446): pid: 18590, tid: 18665, name: Thread-5992 >>> com.plangtongs.skyheroes <<<
I/DEBUG ( 446): signal 7 (SIGBUS), code 1 (BUS_ADRALN), fault addr 0x96d62c87
I/DEBUG ( 446): r0 95213588 r1 96d62c87 r2 00000008 r3 00000000
I/DEBUG ( 446): r4 00000000 r5 00000000 r6 995ef788 r7 95213588
I/DEBUG ( 446): r8 96d62c87 r9 952136c0 sl 96d62c7f fp 00000001
I/DEBUG ( 446): ip 95213580 sp 95213514 lr 9ac016b3 pc 9abff36e cpsr 800f0030
I/DEBUG ( 446):
I/DEBUG ( 446): backtrace:
I/DEBUG ( 446): #00 pc 0144336e /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (cat::SecureEqual(void const_, void const_, int)+27)
I/DEBUG ( 446): #1 pc 014456af /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (cat::AuthenticatedEncryption::Decrypt(unsigned char_, unsigned int&)+282)
I/DEBUG ( 446): #2 pc 0143e461 /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (RakNet::ReliabilityLayer::HandleSocketReceiveFromConnectedPlayer(char const_, unsigned int, RakNet::SystemAddress&, DataStructures::ListRakNet::PluginInterface2
&, int, RakNet::RakNetSocket2_, RakNet::RakNetRandom_, unsigned long long, RakNet::BitStream&)+184)
I/DEBUG ( 446): #3 pc 01434237 /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (RakNet::ProcessNetworkPacket(RakNet::SystemAddress, char const_, int, RakNet::RakPeer_, RakNet::RakNetSocket2_, unsigned long long, RakNet::BitStream&)+150)
I/DEBUG ( 446): #4 pc 0143648f /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (RakNet::RakPeer::RunUpdateCycle(RakNet::BitStream&)+98)
I/DEBUG ( 446): #5 pc 0142edb3 /data/app/com.plangtongs.skyheroes-1/lib/arm/libUE4.so (RakNet::UpdateNetworkLoop(void_)+62)
I/DEBUG ( 446): #6 pc 00012f93 /system/lib/libc.so (__pthread_start(void
)+30)
I/DEBUG ( 446): #7 pc 00011057 /system/lib/libc.so (__start_thread+6)

@larku
Copy link

larku commented Mar 4, 2016

I've recently encountered the exact same issue.

I've replaced the offending method cat::SecureEqual with the implementation below - note this may not be as secure so use at your own risk..

Note: the secureEquals method is used to eliminate timing attacks ( https://www.google.com.au/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=secure%20equals%20timing%20attacks ) as such please asses if this is appropriate for your purposes.

bool SecureEqual(const void *vA, const void *vB, int bytes)
{
      size_t i;
      const unsigned char *a = (const unsigned char *)vA;
      const unsigned char *b = (const unsigned char *)vB;
      unsigned char x = 0;

      for (i = 0; i < bytes; i++)
          x |= a[i] ^ b[i];

      return x == 0;
}

This will be added to my larku fork sometime over the next two/three weeks. This will become the default implementation for Android unless someone has a fix for the current implementation (I don't have time to analyse why the current implementation fails).

More info: As I understand this new implementation can be optimised by the compiler and possibly not be as secure (with respect to timing attacks). The idea is to always compare the entire set of bytes and not stop once a difference is found (can indicate based on time where in the bytes the comparison failed which leads to a way to discover a match).

Another method you could replace this with (which is likely more costly) is to do something simple like this pseudo code:

bool SecureEqual(const void *vA, const void *vB, int bytes)
{
      return sha256(vA) == sha256(vB); 
}

Since the bytes actually being compared will be completely different for every input value, timing attacks do not give any useful information (so I understand).

I'm not a security expert so please take my advice with this in mind.

Let me know if this fixes things for you also.

@kochol
Copy link
Author

kochol commented Mar 5, 2016

Thank you for your quick and complete answer.

I write only return true and it works. It seems this code is not important in client side am I right?

@larku
Copy link

larku commented Mar 5, 2016

Hi @kochol,

SecureEqual is called (indirectly) in many places, it's used in KeyAgreementInitiator and KeyAgreementResponder and AuthenticatedEncryption The AuthenticatedEncryption is used as part of the SecurityLayer which is the heart of the encryption layer.

I'd suggest that returning true here likely makes things very insecure (if not totally insecure).

You'd be far better off using this replacement for SecureEqual()

bool SecureEqual(const void *vA, const void *vB, int bytes)
{
      size_t i;
      const unsigned char *a = (const unsigned char *)vA;
      const unsigned char *b = (const unsigned char *)vB;
      unsigned char x = 0;

      for (i = 0; i < bytes; i++)
          x |= a[i] ^ b[i];

      return x == 0;
}

I believe that would be a far safer option. It's tested and working (in terms of returning correct results) and honours the contract of the original implementation (only returns true when equal).

@kochol
Copy link
Author

kochol commented Mar 5, 2016

Hi @larku

What about the code here.
Is this code good?
https://github.com/catid/libcat/blob/master/SecureEqual.cpp

@larku
Copy link

larku commented Mar 5, 2016

That seems to be identical to the original implementation bundled with RakNet (except for that final line before the return statement).

I'd assume this will fail exactly like the original implementation.

The implementation I suggested you use is reasonably common in crypto libraries so I'd assume it's likely an ok candidate (see here: https://cryptocoding.net/index.php/Coding_rules ).

Do you have a reason to avoid the suggested replacement?

@kochol
Copy link
Author

kochol commented Mar 5, 2016

@larku Thank you for your help.
No I don't have any replacement but I enjoy chatting with you :D
I used your code and it works.

@larku
Copy link

larku commented Mar 5, 2016

Hi @kochol,

Excellent :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants