Off-the-Record Messaging plugin #1387
Conversation
You can start with unbreaking the build :) |
Codecov Report
@@ Coverage Diff @@
## master #1387 +/- ##
===========================================
- Coverage 40.89% 27.66% -13.24%
===========================================
Files 109 110 +1
Lines 21761 22381 +620
===========================================
- Hits 8900 6192 -2708
- Misses 12861 16189 +3328
Continue to review full report at Codecov.
|
|
Hmm, both appveyor failures are python not working on i386, which might not be related to this plugin as it doesn't use python: Looking into writing tests now. |
|
Yep, https://cygwin.com/ml/cygwin/2017-03/msg00281.html reports the same issue about python on 32 bit cygwin. |
mmilata
force-pushed
the
otr
branch
3 times, most recently
from
ccbf55a
to
49d1c8d
Compare
mmilata
force-pushed
the
otr
branch
3 times, most recently
from
cc0abde
to
f581b1b
Compare
Add https://github.com/mmilata/znc-otr (rev 84730d4) to the build tree. Detect libotr in configure script
|
Added a basic test. The ASAN build fails due to a memory leak. Unfortunately the LeakSanitizer output is not very informative and I haven't been able to produce one that has function names instead of addresses. Any idea how to debug this? |
Random guess: It tries to look up symbols, but at this time all the needed shared objects are already unloaded. So a random idea would be to remove all |
| @@ -83,7 +83,8 @@ externalproject_add(inttest_bin | |||
| "-DGTEST_ROOT:path=${GTEST_ROOT}" | |||
| "-DGMOCK_ROOT:path=${GMOCK_ROOT}" | |||
| "-DZNC_BIN_DIR:path=${CMAKE_INSTALL_FULL_BINDIR}" | |||
| "-DQt5_HINTS:path=${brew_qt5}") | |||
| "-DQt5_HINTS:path=${brew_qt5}" | |||
| "-DWITH_OTR:bool=${OTR_FOUND}") | |||
There was a problem hiding this comment.
For the test, we can assume that all dependencies are available.
There was a problem hiding this comment.
That will make the test fail on cygwin/appveyor where non-ancient version of libotr is not available.
| Z; | ||
| ircd1.RelayPrivMsg(ircd2, ":user!user@user/test ", IsOtrCiphertext); | ||
| Z; | ||
| ircd1.RelayPrivMsg(ircd2, ":user!user@user/test ", IsOtrCiphertext); |
There was a problem hiding this comment.
This sequence of relays looks magical...
There was a problem hiding this comment.
Yeah ... couldn't think of less magical way to let the two znc instances exchange messages until the handshake is done.
| ircd1.RelayPrivMsg(ircd2, ":user!user@user/test ", IsOtrCiphertext); | ||
| Z; | ||
| } | ||
| for (int i = 0; i < 3; i++) { |
There was a problem hiding this comment.
That's the number of lines/messages sent in each direction during the shared authentication. Same problem as above.
|
I tried that "remove This is an attempt at getting better backtraces: psychon@18ae57b |
|
A random idea: maybe output can be better, if to upgrade Clang from 3.5.0 to something newer. |
|
Tried with Clang 3.8 but the output seems to be the same. |
|
@psychon hmm ... looking at libotr source, it seems that the memory allocated in |
|
Can we fix the library and use the fixed version in CI? |
|
Guess I can give it a shot. Also looks like the leak is reported already. |
|
https://bugs.otr.im/lib/libotr/commits/master + the activity on the report linked to above Looks like libotr is dead upstream? |
|
It's fairly feature-complete but yeah, there seems to be no recent active development ... I'm having trouble making ASan work on my devel system but I've been able to use Valgrind, I assume there will be some overlap with the leaks found by ASan: http://b42.srck.net/znc-otr-leaks.txt |
|
I tried to find a OTR sample and then tried to fix its memory leaks. I guess upstream does not have much interest in this and so I'll just leave this patch here (and point out that this is a derived work of libotr and so the GPL and the LGPL apply to the patch, i.e. it can be picked up and sent upstream). diff --git a/src/context.c b/src/context.c
index 44d8b86..ca42971 100644
--- a/src/context.c
+++ b/src/context.c
@@ -511,10 +511,12 @@ int otrl_context_forget(ConnContext *context)
free(context->accountname);
free(context->protocol);
free(context->smstate);
+ free(context->context_priv);
context->username = NULL;
context->accountname = NULL;
context->protocol = NULL;
context->smstate = NULL;
+ context->context_priv = NULL;
/* Free the application data, if it exists */
if (context->app_data && context->app_data_free) {
diff --git a/src/instag.c b/src/instag.c
index cccd94f..609bced 100644
--- a/src/instag.c
+++ b/src/instag.c
@@ -205,10 +205,7 @@ otrl_instag_t otrl_instag_get_new()
otrl_instag_t result = 0;
while(result < OTRL_MIN_VALID_INSTAG) {
- otrl_instag_t * instag = (otrl_instag_t *)gcry_random_bytes(
- sizeof(otrl_instag_t), GCRY_STRONG_RANDOM);
- result = *instag;
- gcry_free(instag);
+ gcry_randomize((unsigned char *) &result, sizeof(result), GCRY_STRONG_RANDOM);
}
return result;
diff --git a/tests/regression/client/client.c b/tests/regression/client/client.c
index e72b661..0afa905 100644
--- a/tests/regression/client/client.c
+++ b/tests/regression/client/client.c
@@ -76,6 +76,11 @@ static OtrlFragmentPolicy fragPolicy = OTRL_FRAGMENT_SEND_SKIP;
static const char *protocol = "otr-test";
static const char *alice_name = "alice";
static const char *bob_name = "bob";
+/* Some counters to fix a race condition: A message could be sent, then we
+ * decide to quit and the in-flight message is leaked.
+ */
+static int messages_to_alice_inflight = 0;
+static int messages_to_bob_inflight = 0;
static const char *unix_sock_bob_path = "/tmp/otr-test-bob.sock";
static const char *unix_sock_alice_path = "/tmp/otr-test-alice.sock";
@@ -561,6 +566,7 @@ static int recv_otr_msg(int sock, const char *to, const char *from,
if (new_msg) {
OK(strncmp(omsg->plaintext, new_msg, omsg->plaintext_len) == 0,
"Message exchanged is valid");
+ otrl_message_free(new_msg);
update_msg_counter();
}
} else {
@@ -711,7 +717,7 @@ static void *alice_thread(void *data)
fd = ev[i].data.fd;
event = ev[i].events;
- if (fd == quit_pipe[0]) {
+ if (fd == quit_pipe[0] && messages_to_alice_inflight == 0) {
/* Time to leave. */
goto end;
} else if (fd == alice_sock) {
@@ -722,6 +728,7 @@ static void *alice_thread(void *data)
struct sockaddr_un sun;
/* Connection from Bob, accept it so we can handle it. */
+ len = sizeof(sun);
sock_from_bob = accept(fd, (struct sockaddr *) &sun,
&len);
ret = add_sock_to_pollset(epfd, sock_from_bob,
@@ -738,6 +745,7 @@ static void *alice_thread(void *data)
} else if (event & EPOLLIN) {
(void) recv_otr_msg(sock_from_bob, alice_name, bob_name,
&oinfo);
+ messages_to_alice_inflight--;
}
continue;
} else {
@@ -821,6 +829,7 @@ static void *bob_thread(void *data)
/* No event thus timeout, send message to Alice. */
if (nb_fd == 0) {
+ messages_to_alice_inflight++;
(void) send_otr_msg(sock_to_alice, alice_name, bob_name, &oinfo,
NULL);
continue;
@@ -833,7 +842,7 @@ static void *bob_thread(void *data)
fd = ev[i].data.fd;
event = ev[i].events;
- if (fd == quit_pipe[0]) {
+ if (fd == quit_pipe[0] && messages_to_bob_inflight == 0) {
/* Time to leave. */
goto end;
} else if (fd == bob_sock) {
@@ -844,6 +853,7 @@ static void *bob_thread(void *data)
struct sockaddr_un sun;
/* Connection from Alice, accept it so we can handle it. */
+ len = sizeof(sun);
sock_from_alice = accept(fd, (struct sockaddr *) &sun,
&len);
ret = add_sock_to_pollset(epfd, sock_from_alice,
@@ -859,6 +869,7 @@ static void *bob_thread(void *data)
} else if (event & EPOLLIN) {
(void) recv_otr_msg(sock_from_alice, bob_name,
alice_name, &oinfo);
+ messages_to_bob_inflight--;
}
continue;
} else {
@@ -1015,7 +1026,8 @@ static int init_client(void)
int ret;
/* Init libgcrypt threading system. */
- gcry_control(GCRYCTL_SET_THREAD_CBS, &gcry_threads_pthread);
+ // Docs say this is obsolete since version 1.6
+ //gcry_control(GCRYCTL_SET_THREAD_CBS, &gcry_threads_pthread);
/* Init OTR library. */
OTRL_INIT;
@@ -1151,6 +1163,11 @@ int main(int argc, char **argv)
run();
+ otrl_userstate_free(user_state);
+ free(opt_instag_path);
+ free(opt_key_path);
+ free(opt_key_fp_path);
+
return exit_status();
error: |
|
@psychon thanks for looking into this! I threw your patch in the upstream direction: https://bugs.otr.im/lib/libotr/issues/90#note_449 |
|
If that patch works, may you install the fixed libotr to travis to fix the test? Meanwhile, I'll start reviewing the module itself, and will try to remove the sequences of magic numbers of PRIVMSG from test. |
| virtual ~COtrTimer() {} | ||
|
|
||
| protected: | ||
| virtual void RunJob(); |
| COtrTimer(CModule* pModule, unsigned int uInterval) | ||
| : CTimer(pModule, uInterval, /*run forever*/ 0, "OtrTimer", | ||
| "OTR message poll") {} | ||
| virtual ~COtrTimer() {} |
There was a problem hiding this comment.
Not necessary, CTimer has it virtual already anyway
| friend class COtrGenKeyJob; | ||
|
|
||
| private: | ||
| static OtrlMessageAppOps m_xOtrOps; |
There was a problem hiding this comment.
Is this supposed to be shared between different users who loaded otr module?
There was a problem hiding this comment.
Yes - it's a structure with function pointers that are the same for every user.
There was a problem hiding this comment.
Sorry for stupid question but how does one initialize static const struct member?
There was a problem hiding this comment.
struct foo {
static const int x;
};
const int foo::x = 42;| struct COtrAppData { | ||
| bool bSmpReply; | ||
|
|
||
| COtrAppData() { bSmpReply = false; } |
There was a problem hiding this comment.
You can simplify this to bool bSmpReply = false; and no explicit constructors.
|
|
||
| enum Color { Blue = 2, Green = 3, Red = 4, Bold = 32 }; | ||
|
|
||
| CString Clr(Color eClr, const CString& sWhat) { |
| return SendEncrypted(sTarget, sLine); | ||
| } | ||
|
|
||
| virtual EModRet OnPrivMsg(CNick& Nick, CString& sMessage) { |
There was a problem hiding this comment.
These callbacks are deprecated (though will be supported for a while), the new way is to use CMessage-based callbacks
|
|
||
| CString Clr(Color eClr, const CString& sWhat) { | ||
| if (eClr == Bold) { | ||
| return CString("\x02") + sWhat + CString("\x02"); |
| CString sMsg = | ||
| Clr(Red, "WARNING:") + " The log module is loaded. Type "; | ||
| if (bUserMod) { | ||
| sMsg += Clr(Bold, "/msg *status UnloadMod log") + " "; |
There was a problem hiding this comment.
StatusPrefix is not always *
| if (bUserMod) { | ||
| sMsg += Clr(Bold, "/msg *status UnloadMod log") + " "; | ||
| if (bNetworkMod) { | ||
| sMsg += "and "; |
There was a problem hiding this comment.
Such constructed message would be hard to translate
|
|
||
| // Warn if we are not an administrator | ||
| // We should check if we are the only administrator but the user map may | ||
| // not be |
There was a problem hiding this comment.
New lines in comments caused by format settings?
|
Is it a good idea to have otr (which is intended to provide end-to-end security) be terminated by a middleman like ZNC? |
|
That should be the choice of the user. Having it in ZNC is more convenient than configuring it separately on several clients. |
| // Initialize libotr if needed | ||
| static bool otrInitialized = false; | ||
| if (!otrInitialized) { | ||
| OTRL_INIT; |
There was a problem hiding this comment.
This will cause problems.
- https://bugs.otr.im/lib/libotr/blob/master/src/mem.c#L158 can potentially conflict with other stuff in the process which happens to also use gcrypt. ZNC itself does not use gcrypt, but some external modules can.
- If you unload all instances of otr module, then load it again, things are likely to break, because some address in newly loaded .so can change. Modperl suffers from similar issue, but modperl is at least not loadable/unloadable by users.
| return CONTINUE; | ||
| } | ||
|
|
||
| const char* accountname = GetUser()->GetUserName().c_str(); |
There was a problem hiding this comment.
According to libotr docs, accountname should also contain network name; otherwise someone from efnet can impersonate someone with the same nick on freenode. Or it's not a problem here because the module is for one network?
There was a problem hiding this comment.
Shouldn't be a problem, each network has its own nick (context) database. Also each network has its own private key:(
|
|
||
| void CmdIgnore(const CString& sLine) { | ||
| if (sLine.Token(1).Equals("")) { | ||
| PutModuleBuffered("OTR is disabled for following nicks:"); |
There was a problem hiding this comment.
m_vsIgnored should be a set, not a vector
| table.AddColumn("Peer"); | ||
| table.AddColumn("State"); | ||
| table.AddColumn("Fingerprint"); | ||
| table.AddColumn("Act"); |
There was a problem hiding this comment.
What is act? acting?
Reading code below, it's "active", but user probably doesn't read the code
| void WarnIfLoggingEnabled() { | ||
| assert(GetNetwork()); | ||
| bool bUserMod = GetUser()->GetModules().FindModule("log"); | ||
| bool bNetworkMod = GetNetwork()->GetModules().FindModule("log"); |
| } | ||
| } | ||
|
|
||
| static CString HumanFingerprint(Fingerprint* fprint) { |
There was a problem hiding this comment.
HumanReadableFingerprint()
I thought it's fingerprint of a human.
| DoSMP(ctx, "", sSecret); | ||
| } | ||
|
|
||
| void CmdAuthQ(const CString& sLine) { |
There was a problem hiding this comment.
I see some instructions to run Auth command, but never AuthQ
There was a problem hiding this comment.
Users never look at integration test for documentation.
Instructions to run Auth command are sent to user by this module itself, in certain cases.
I'm sorry but it seems I won't be able to commit significant amount of time into this PR. I think I'd be able to address the minor issues found here but can't do bigger stuff like fixing libotr memleaks. I don't even really use znc with the module anymore:/ The time you guys invested in reviewing this PR is much appreciated, however if it looks it's going to cause problems in the future maybe it would be better to drop the PR before further time is wasted on it. What do you think? |
This is even bigger problem than OTRL_INIT. That one can be worked around by making module global (with certain other changes) and declaring that it can't work together with some other modules. But this module may require maintenance even after merging the PR. If noone uses it, it'll be very hard to do. Well, at least I learnt a bit about libotr, and you learnt a bit about C++ :) |
Hi, some time ago I wrote OTR encryption plugin for ZNC and am wondering if you'd like to include it upstream. If yes is there anything that needs to be changed?