Applied fixes for crypto in RX-to-TX cloning #365

ethouris · 2018-04-30T11:14:12Z

Fixes:

The HaiCrypt_Create function was extracted the major part, common for RX and TX direction into a new function sHaiCrypt_PrepareHandle.
Added a function that extracts the HaiCrypt_Cfg type object from the RX crypto after it is initialized by KMREQ message.
Added a function, hcrypt_Tx_CloneKey, which is almost a full copy of hcrypt_Tx_Rekey, except that instead of generating a random byte sequence for SEK and SALT, it copies them from the given RX crypto. The KEK generation based on SALT and passphrase, as well as all other initialization activities remain as they were in Rekey function.
The HaiCrypt_Clone function for a case when target is a TX crypto, uses the following method:
- Creates the extracted configuration from RX
- Calls sHaiCrypt_PrepareHandle, which is the most common part with HaiCrypt_Create, except the last part that is direction-specific
- The rest of TX crypto initialization part was copied from that in HaiCrypt_Clone, with the exception that the hcrypt_Tx_Rekey call was replaced with hcrypt_Tx_CloneKey
Fixed in sendKeysToPeer so that this may be called also when the agent is HSD_RESPONDER, as long as the regen is REGEN_KM, which means that this is done while transmission, not during the handshake
When running in handshake mode (bidirectional), after successful processing into RX and cloning into TX, the incoming KMX message is recorded in m_SndKmMsg so that it can be used later to craft KMRSP message without having the incoming KMREQ at hand

…ing KMX retries when crafting integrated handshake

…l KMX. This will be also reused to craft out-of-band HS with KMRSP

rndi · 2018-05-23T20:41:06Z

haicrypt/hcrypt_ctx_tx.c

@@ -100,6 +100,67 @@ int hcryptCtx_Tx_Rekey(hcrypt_Session *crypto, hcrypt_Ctx *ctx)
 	return(0);
 }

+int hcryptCtx_Tx_CloneKey(hcrypt_Session *crypto, hcrypt_Ctx *ctx, const hcrypt_Session* cryptoSrc)


Tabs vs spaces throughout this function.

rndi · 2018-05-23T20:41:50Z

haicrypt/hcrypt_ctx_tx.c

@@ -129,13 +190,16 @@ int hcryptCtx_Tx_Refresh(hcrypt_Session *crypto)

 	/* Generate new SEK */
 	new_ctx->sek_len = new_ctx->cfg.key_len;
+
+    HCRYPT_LOG(LOG_DEBUG, "refresh/generate SEK. salt_len=%d sek_len=%d\n", (int)new_ctx->salt_len, (int)new_ctx->sek_len);


Tabs vs spaces

rndi · 2018-05-23T20:42:12Z

haicrypt/hcrypt_ctx_tx.c

@@ -271,6 +335,10 @@ int hcryptCtx_Tx_ManageKM(hcrypt_Session *crypto)

 	ASSERT(NULL != ctx);

+    HCRYPT_LOG(LOG_DEBUG, "KM[%d] KEY STATUS: pkt_cnt=%u against ref.rate=%u and pre.announce=%u\n",


tabs vs spaces

rndi · 2018-05-23T21:43:46Z

srtcore/crypto.h

@@ -118,16 +118,26 @@ class CCryptoControl

    const unsigned char* getKmMsg_data(size_t ki) const { return m_SndKmMsg[ki].Msg; }
    size_t getKmMsg_size(size_t ki) const { return m_SndKmMsg[ki].MsgLen; }
-    bool getKmMsg_needSend(size_t ki) const
+    bool getKmMsg_needSend(size_t ki, bool runtime) const


'runtime' is always false?

The plan was to use these functions also in CCryptoControl::sendKeysToPeer and then move this whole function to CUDT in order to avoid reverse-call to CUDT::sendSrtMsg (another refactoring put aside). As this function is in CCryptoControl, it can use direct access to the "KmMsg" things instead of this function, but when moved to CUDT it will have to use them, and then the runtime parameter would have to be true. I could remove this parameter and associated code, but I really wouldn't like to lose it because if there happens next time the need to use them at the "during transmission" stage, the developer may forget the correct use of it in this situation and cause another hard to find bug.

OK. Could you please add a comment explaining the function of the 'runtime' parameter for now? Its name alone does not give enough insight to what the parameter does or how it needs to be configured.

rndi · 2018-05-23T21:43:57Z

srtcore/crypto.h

    }

-    void getKmMsg_markSent(size_t ki)
+    void getKmMsg_markSent(size_t ki, bool runtime)


'runtime' is always false?

rndi · 2018-05-23T22:10:21Z

haicrypt/hcrypt.c

-int HaiCrypt_Create(const HaiCrypt_Cfg *cfg, HaiCrypt_Handle *phhc)
+static const char* DumpCfgFlags(int flags)
+{
+    static char buf[4096];


Please avoid returning non-const static buffers from a function. There is really no need for this: just pass destination buffer and its length as arguments.

rndi · 2018-05-24T14:28:15Z

haicrypt/hcrypt.c

@@ -41,18 +41,16 @@ int HaiCrypt_SetLogLevel(int level, int logfa)

 #if ENABLE_HAICRYPT_LOGGING

-static const char* DumpCfgFlags(int flags)
+static const char* DumpCfgFlags(int flags, char* buf)


The buffer length needs to be passed as well and checked for buffer overruns on buffer access.

This is not a public function, and it's predicted to be used only for logging purposes, and logging in this FA must be turned on explicitly by a distinct option (that is, this function will not be used, unless someone makes a special build for development purposes only, with a special option, that is also warned about security concerns when used).

Ok, as this is shared with #359, the problem will be solved there, and this PR remerged after this fix, so this problem should disappear.

StephaneStH

I guess the comments below were never sent out... Let's see if this triggers the sending....

StephaneStH · 2018-05-23T15:11:27Z

haicrypt/hcrypt.c

+    hcrypt_Session *crypto;
+    HaiCrypt_CryptoDir tx = (HaiCrypt_CryptoDir)(HAICRYPT_CFG_F_TX & cfg->flags);
+
+    *phhc = NULL;


Since we have assert on cfg not being NULL, maybe an assert for phhc before writing to it would be good too.

Right. Moreover, the assertion for cfg is kinda useless if done after the use. Fixing.

StephaneStH · 2018-05-23T15:13:11Z

haicrypt/hcrypt.c

+        HCRYPT_LOG(LOG_ERR, "invalid secret passphrase length (%d)\n", (int)cfg->secret.len);
+        return(-1);
+    } else if ((HAICRYPT_SECTYP_PRESHARED == cfg->secret.typ)
+            &&  (16 != cfg->key_len)	/* SEK length */


Unless I am missing something, this test appears redundant with what is at line 221...

This is an old code, so I paid no attention on it. Yes, it looks like this condition will be always false (stating that any true part in the key length checking has been caught in this previous one). Removing.

StephaneStH · 2018-05-23T15:34:36Z

haicrypt/hcrypt.c

-    } else { /* Receiver */
+        mem_buf = (unsigned char *)cryptoClone;
+        mem_buf += sizeof(*cryptoClone);
+        memset(cryptoClone, 0, sizeof(*cryptoClone));


memset looks superfluous since there is a memcpy over the same exact area right after.

Right. memset probably was earlier for something else, or copying was not exactly to the whole object.

StephaneStH · 2018-05-23T22:21:02Z

haicrypt/hcrypt.c

+    size_t maxlen = 1024 - (pbuf - buf_secret) - 5;
+    if (cfg->secret.len > maxlen)
+    {
+        memcpy(pbuf, (char*)cfg->secret.str, maxlen);


The cast here is not needed, you have a similar memcpy a few lines later without it.

This function has been rewritten to C++, so it's no longer a problem.

StephaneStH · 2018-05-24T14:07:52Z

haicrypt/hcrypt.c

@@ -41,18 +41,16 @@ int HaiCrypt_SetLogLevel(int level, int logfa)

 #if ENABLE_HAICRYPT_LOGGING

-static const char* DumpCfgFlags(int flags)
+static const char* DumpCfgFlags(int flags, char* buf)


The idea was I believe to pass a buffer AND it's length/size. DumpCfgFlags has now no idea of the limits when writing to that buf.

No, the idea was to pass the buffer that is always large enough to hold a string spanning for one line. Yes, it looks ugly, so the whole function has been rewritten to C++, it uses std::ostringstream and so the buffer size checking is off head.

…crypt only once per KMX period. Removed unnecessary decrypt log errors.

…ore logs around legacy HSREQ

Mikołaj Małecki added 2 commits April 30, 2018 13:02

Applied fixes for crypto in RX-to-TX cloning

02ff2ad

Fixed bug: incorrect state after wrong password check condition

03cb74d

ethouris mentioned this pull request May 3, 2018

Fixes for initial and refreshing KMX process #352

Closed

Mikołaj Małecki added 4 commits May 8, 2018 13:01

Fixed bug in PBKEYLEN default value check. Fixed problem with decreas…

19ef48f

…ing KMX retries when crafting integrated handshake

Fixed correct use of extracting KMX to attach to handshake

b521771

Fixed problem of memory leak by multiple cloning TX to RX

861d11e

Recording incoming KMX message so that the reverse-KMX looks like rea…

9b3fc30

…l KMX. This will be also reused to craft out-of-band HS with KMRSP

ethouris mentioned this pull request May 18, 2018

Fixed problems in handshake handling #361

Merged

ethouris added the Priority: High label May 18, 2018

rndi reviewed May 23, 2018

View reviewed changes

rndi mentioned this pull request May 23, 2018

Fixed wrong condition check on uninitialized PBKEYLEN when NOSECRET #370

Closed

rndi reviewed May 23, 2018

View reviewed changes

Fixes after review

d103fcb

ethouris mentioned this pull request May 24, 2018

Adding a logging system for haicrypt #359

Merged

rndi reviewed May 24, 2018

View reviewed changes

StephaneStH suggested changes May 24, 2018

View reviewed changes

Mikołaj Małecki and others added 9 commits May 25, 2018 08:41

Prepared for remerge (UNSTABLE VERSION)

56dd5d4

Rebased

afae3e1

Fixes after review

4fa2525

Added dox explaining the new 'runtime' parameter

db39e59

Fixed common style for sibling instructions

64e91ef

Preventing decrypt if state is not SECURED. Error reporting on non-de…

2aa5175

…crypt only once per KMX period. Removed unnecessary decrypt log errors.

Blocked sending prevention when KMX failed. Allowed to send scrambled.

8cb1674

Fixed: on KMX failure and have passphrase, create fake crypto

6c7d422

Fixed bug: save the HS SIDE once determined in createCrypter. Added m…

70a8f62

…ore logs around legacy HSREQ

rndi merged commit febe51f into Haivision:master Jun 12, 2018

ethouris deleted the dev-fix-rendezvous-processing-crypto branch January 23, 2019 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Applied fixes for crypto in RX-to-TX cloning #365

Applied fixes for crypto in RX-to-TX cloning #365

ethouris commented Apr 30, 2018 •

edited

Loading

rndi May 23, 2018

rndi May 23, 2018

rndi May 23, 2018

rndi May 23, 2018

ethouris May 24, 2018

rndi May 24, 2018

ethouris May 24, 2018

rndi May 23, 2018

rndi May 23, 2018

rndi May 24, 2018

ethouris May 24, 2018

ethouris May 25, 2018

StephaneStH left a comment

StephaneStH May 23, 2018

ethouris May 25, 2018

StephaneStH May 23, 2018

ethouris May 25, 2018

StephaneStH May 23, 2018

ethouris May 25, 2018

StephaneStH May 23, 2018

ethouris May 25, 2018

StephaneStH May 24, 2018

ethouris May 25, 2018

		@@ -271,6 +335,10 @@ int hcryptCtx_Tx_ManageKM(hcrypt_Session *crypto)

		ASSERT(NULL != ctx);

		HCRYPT_LOG(LOG_DEBUG, "KM[%d] KEY STATUS: pkt_cnt=%u against ref.rate=%u and pre.announce=%u\n",

Applied fixes for crypto in RX-to-TX cloning #365

Applied fixes for crypto in RX-to-TX cloning #365

Conversation

ethouris commented Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephaneStH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethouris commented Apr 30, 2018 •

edited

Loading