Hi, Here's the summary of the previous IRC meeting / sprint.
Next meeting will be on _Tuesday_ 26th July at 17:00 UTC. In this meeting we select methods/tools for taking "behavioral diffs", which help ensure the PolarSSL refactoring patches don't change any functionality. This meeting will be followed by a standard meeting/sprint on Thursday 28th July at 17:00 UTC, where the remaining verification functions will be reviewed. --- COMMUNITY MEETING Place: #openvpn-devel on irc.freenode.net List-Post: openvpn-devel@lists.sourceforge.net Date: Thursday, 21st July 2011 Time: 17:00 UTC Planned meeting topics for this meeting were on this page: <https://community.openvpn.net/openvpn/wiki/Topics-2011-07-21> Next meeting will be announced in advance, but will be on the same weekday and at the same time. Your local meeting time is easy to check from services such as <http://www.timeanddate.com/world clock> or with $ date -u SUMMARY andj, jamesyonan and mattock participated in this meeting. -- This meeting was sprint, where Adriaan's (andj's) PolarSSL patches were reviewed, fixed and ACKed on the fly. The sprint focused on the "Verification functions" patchset: <https://community.openvpn.net/openvpn/wiki/PolarSSLintegration?version=6#Verificationfunctions> The above page also shows the ACK status of patches (after the meeting). The majority of patches were not ACKed on the fly, as verifying that they don't change any functionality was difficult using standard syntactic diffs. However, at the end of the meeting andj came up with a clever script to generate diffs that make reviewing easier: #!/bin/bash git diff $1 $2 >/tmp/difftmp123.txt 22:27:38 cat /tmp/difftmp123.txt |grep "^-" |sed s/^-// >/tmp/removed123.txt 22:27:38 cat /tmp/difftmp123.txt |grep "^+" |sed s/^+// >/tmp/added123.txt 22:27:38 diff /tmp/removed123.txt /tmp/added123.txt -u Jamesyonan promised to take a look at other, more fancy alternatives, perhaps something along the lines of SymDiff: <http://research.microsoft.com/en-us/projects/symdiff> These tools will be reviewed next Tuesday, and patch review will resume on Thursday (see above). If you have any comments regarding any of the patches, please chime in. If there are no complaints, the ACKed patches will be merged to main Git repository soon. --- Full chatlog as an attachment -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock
andj 20:00:43 Hi jamesyonan 20:00:50 hi mattock 20:01:04 hi jamesyonan jamesyonan 20:01:33 hi guys mattock 20:01:53 topic list here: https://community.openvpn.net/openvpn/wiki/Topics-2011-07-21 vpnHelper 20:01:55 Title: Topics-2011-07-21 â OpenVPN Community (at community.openvpn.net) andj 20:02:08 dazo asked us to review the SSL verification patches today mattock 20:02:14 the actual patch list for today is here: https://community.openvpn.net/openvpn/wiki/PolarSSLintegration#Verificationfunctions vpnHelper 20:02:17 Title: PolarSSLintegration â OpenVPN Community (at community.openvpn.net) andj 20:02:32 That means that we skip the separation patches until he is back/or next week depending on what we decide later today 20:02:43 The idea behind these patches 20:03:01 is that SSL functionality has already been split into ssl_backend.h, and ssl_openssl.c 20:03:18 in the previous set 20:03:24 just like for crypto_backend.h and crypto_openssl.c 20:03:40 This patch set builds on that 20:03:56 and separates out the verification functions 20:04:04 which were also originally defined in ssl.c 20:04:16 This was done to clean up the verification code, allowing easier hooks to be created in the future 20:04:54 so, without further ado, anyone ready for the first patch? 20:05:10 jamesyonan 20:05:44 so the first patch is just moving stuff to ssl_common.h ? andj 20:05:54 Note that, although it's not part of this series, https://github.com/andj/openvpn-ssl-refactoring/commit/46e7d0b6ae89634e70686bf48bfcdca07249f829 20:05:55 vpnHelper 20:05:57 Title: Commit 46e7d0b6ae89634e70686bf48bfcdca07249f829 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 20:06:11 is the basis, where ssl_verify* files are created In that patch they're just stubs though 20:06:23 https://github.com/andj/openvpn-ssl-refactoring/commit/1e3d80aeafa910a21bf9fe4e23c59392ea6fc551 is the patch jamesyonan was refering to 20:06:45 vpnHelper 20:06:47 Title: Commit 1e3d80aeafa910a21bf9fe4e23c59392ea6fc551 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 20:06:52 and that's just moving stuff around L'utente dazo è ora conosciuto come dazo_afk 20:07 andj 20:07:47 So that required structures are available from the ssl_verify files jamesyonan 20:08:30 I wish the diff could be smarter and simply indicate that text in the file was moved rather than making it indistinguishable from a large patch that changes many lines of code andj 20:08:39 agree 100% That would have made my life much easier during porting from 2.1 to 2.3 too 20:09:09 Anyway, had a chance to verify that there were no changes? 20:11:26 jamesyonan 20:11:36 I'm certainly fine with the patch if it's just moving stuff around andj 20:11:50 ok, moving on to the next one: https://github.com/andj/openvpn-ssl-refactoring/commit/365d2319d95f4374072a2b6ea49b1b6c472fbb39 vpnHelper 20:11:51 Title: Commit 365d2319d95f4374072a2b6ea49b1b6c472fbb39 to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 20:12:26 That one extracts the client_config_dir_exclusive stuff to its own function within verification, taking it out of key_method_2_read Is that one ok with you? 20:14:12 mattock 20:14:38 andj: so both 1e3d80a and46e7d were ACKed? andj 20:15:03 mattock: we'll leve 46e7 for next time mattock 20:15:07 ok andj 20:15:12 as it's part of a different series But I don't expect any problems with that one 20:15:22 jamesyonan 20:17:07 yes, 365d looks reasonable (I wish there was a coverity-like analysis tool that could verify that a given refactoring doesn't change functionality) andj 20:17:37 All I can say is that I checked while I was working on it, but that doesn't give a 100% guarantee More eyes on a piece of code are the best solution for now 20:17:52 https://github.com/andj/openvpn-ssl-refactoring/commit/bbe117b0217180718f9d84ed21c149b0d0f035ad 20:18:00 vpnHelper 20:18:02 Title: Commit bbe117b0217180718f9d84ed21c149b0d0f035ad to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 20:18:10 This one moves the cert hash verification again, pretty straightforward move 20:19:07 Do note that cert_has_remember has its prototype definition in ssl_verify_backend.h 20:19:25 That's because it is used as a support function by the backend, not because it is defined there 20:19:46 But that'll come in a later patch 20:19:58 mattock 20:24:40 jamesyonan: any thoughts? ... 20:27:50 andj 20:27:58 jamesyonan 20:28:41 honestly, I don't think anyone can look at these patches and verify that they don't change functionality isn't there any tool that we can use to assist in this? 20:28:59 L'utente psha si è disconnesso (Quit: leaving) 20:29 mattock 20:29:24 I'll take a look at google maybe we should give conditional "if this does not change functionality" ACKs today? 20:32:34 and try to find tools/techniques that could verify that 20:32:48 later 20:32:51 andj 20:33:04 That sounds like a good idea I'm googling in the background 20:33:12 jamesyonan 20:33:12 andj: don't get me wrong -- I very much appreciate your work -- I'm just feeling daunted by my inability to look at these patches and verify that they don't change any functionality without better tools mattock 20:34:00 jamesyonan: what if you look at the patches an ACK them conditionally as ^^^ andj 20:34:06 Don't worry, I understand, and I've spent quite a lot of time trying to fix it this way But just don't know how to do it differently using normal diffs 20:34:29 jamesyonan 20:34:57 this stuff obviously lies at the core of the SSL verification functionality, so even subtle bugs in refactoring could introduce vulnerabilities we need to mitigate this risk somehow 20:35:09 mattock 20:36:25 any ideas if this kind of vulnerabilities would be caught using static analysis (e.g. valgrind/coverity)? jamesyonan 20:37:11 yes, this is clearly a static analysis problem -- the question is whether the static analysis tools support refactoring analysis mattock 20:39:46 I'll try to dig some info on this... jamesyonan 20:41:31 It looks like there's some open source work in this area -- https://developer.mozilla.org/en/Dehydra vpnHelper 20:41:33 Title: Dehydra - MDN Docs (at developer.mozilla.org) jamesyonan 20:43:33 it looks like this sort of thing is sometimes referred to as Differential Static Analysis andj 20:44:28 couldn't we do this a little simpler? Take the code in one file, and the added code, and apply a diff between that? jamesyonan 20:45:11 what we really need is a smarter diff it looks like others are thinking along these lines as well: http://research.microsoft.com/en-us/projects/symdiff/ 20:45:36 vpnHelper 20:45:37 Title: SymDiff - Microsoft Research (at research.microsoft.com) mattock 20:46:37 andj: did you use any refactoring tool for your refactoring? it seems there are some available, which I guess would check that functionality does not change 20:46:52 andj 20:47:03 eclipse refactorings, but they're limited for C mattock 20:48:27 eclipse CDT? andj 20:48:31 yeah mattock 20:48:40 http://r2.ifs.hsr.ch/cdtrefactoring vpnHelper 20:48:43 Title: CDT Refactoring (at r2.ifs.hsr.ch) andj 20:49:00 again, very limited jamesyonan 20:49:41 I don't think we necessarily need a refactoring tool for this -- we just need a smarter diff andj 20:50:01 which, if I look around is near impossible to find jamesyonan 20:50:29 for example if we had a tool that could parse C into (say) semantic trees and then diff at the tree level andj 20:50:38 as it seems to be NP jamesyonan 20:50:56 you mean NP-complete? mattock 20:51:09 it seems this symdiff is available for Linux, too: http://symdiff.com/ vpnHelper 20:51:13 Title: Symdiff | Symbolic Differentiation (at symdiff.com) andj 20:51:13 can't find a reference to it's completeness mattock 20:51:19 unless it's a different symdiff ah, it probably is 20:51:28 jamesyonan 20:53:18 say you break the code into C trees and take the hash of each subtree -- then when you display the diff, any equivalent subtrees are marked as such and collapsed complexity would appear at first glance to be n-squared -- not really a big deal 20:53:54 andj 20:55:17 True, unfortunately, I can't find one Is there a different way that we can approach this? 20:55:40 jamesyonan 20:57:53 I'm torn in the sense that I completely agree that the crypto/SSL stuff should be abstracted, but I'm worried about correctness here The thing to do would be to parse the C code with this http://code.google.com/p/pycparser/ 20:58:21 vpnHelper 20:58:24 Title: pycparser - C parser and AST generator written in Python - Google Project Hosting (at code.google.com) jamesyonan 20:59:17 then do a smart diff by walking the tree andj 20:59:55 The amount of time that we would have to put into that isn't really proportionate though, is it? jamesyonan 21:00:21 the diff would collapse all repetitions of the same subtree mattock 21:01:12 jamesyonan: is this something you could do easily? jamesyonan 21:02:12 It's hard to believe no one has done this yet -- it seems like a classic kind of computer science problem how do other projects handle this in the sense of having a large patch that i 21:05:40 ... influences security in a fundamental way and is mostly about refactoring and abstraction layers 21:06:01 mattock 21:06:35 pycparser looks interesting, even has automated unit testing discovery... which would also help catch potential refactoring issues jamesyonan 21:07:22 I would love to work on this, but I suspect that this would be the beginning of a new open source project mattock 21:07:30 jamesyonan 21:07:49 semantic diff for C mattock 21:08:00 so, what would be the simplest solution in our case... without spending excessive amounts of time on it? jamesyonan: what about andj's earlier suggestion: "couldn't we do this a little simpler? Take the code in one file, and the added code, and apply a diff between that?" 21:09:35 jamesyonan 21:09:57 what if we confine the patches to an alternate branch pending some level of automated verification? mattock 21:10:27 sounds good andj 21:10:47 I disagree I don't expect automated verification to happen any time soon, as it requires tooling that currently just doesn't exist 21:11:13 meaning that these patches could hang in limbo for a long time 21:11:40 jamesyonan 21:12:40 andj: I'm just totally OCD about security andj 21:13:03 So am I, and I agree with you that you want to see clearer proof I just want to find an easier solution 21:13:13 jamesyonan 21:14:12 how about this, I'll take a couple days to look more in depth at automated verification options andj 21:14:27 How about this: I provide two files: functions that were removed, functions that were added and we take a diff between them? 21:14:34 jamesyonan 21:14:40 and get a sense of whether it's feasible at this time to take such an approach mattock 21:15:16 perhaps do both? fallback to andj's suggestion if automated approach fails 21:15:32 jamesyonan 21:15:44 yes, I think that's reasonable mattock 21:16:11 andj: is there anything to review in these patches, besides verifying they don't change functionality? meaning, should we review them at all today? 21:16:49 andj 21:16:53 There are a few places where functionality is refactored in a more fundamental way, which we could look at mattock 21:17:10 let's do those, then, shall we? andj 21:17:14 Trouble is, the point of most of these patches is to not change anything This one shouldn't be a problem: https://github.com/andj/openvpn-ssl-refactoring/commit/77aa7ead6e86082045e5423d88df8cb1d6179efd 21:17:46 vpnHelper 21:17:50 Title: Commit 77aa7ead6e86082045e5423d88df8cb1d6179efd to andj/openvpn-ssl-refactoring - GitHub (at github.com) jamesyonan 21:19:59 so in this, X509_NAME_CHAR_CLASS is being copied from another file I presume? andj 21:20:04 yes gets removed in a later patch, but is required in two places for now 21:20:22 jamesyonan 21:20:45 sure, that's fine andj 21:21:23 This one is more complex, but needs to be looked at by hand anyway: https://github.com/andj/openvpn-ssl-refactoring/commit/950b2182d8846d794ca1339b8d20ad7532801c5f vpnHelper 21:21:24 Title: Commit 950b2182d8846d794ca1339b8d20ad7532801c5f to andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 21:21:34 That's because it splits the verify_callback into an SSL-lib specific part 21:21:41 and a non-ssl lib specific part 21:21:50 The lib-specific part is responsible for checking that preverification was ok, and for calculating/storing the hash 21:24:25 as that needs to be done whether pre-verification passes or fails (to not change functionality) 21:24:48 jamesyonan 21:32:33 andj: so is this done so that you can call verify_cert from other places? andj 21:32:44 yes, for example from the polar code verify_cert is generic (or will be in later patches) 21:32:58 and the openssl callback isn't 21:33:07 jamesyonan 21:36:06 so you're saying that "did peer present cert which was signed by our root cert?" check is done differently for PolarSSL code than OpenSSL code? andj 21:36:44 No, the callback has a different signature so the verify_cert function is generic 21:36:56 the same for both Polar and OpenSSL 21:37:05 But "verify_callback (int preverify_ok, X509_STORE_CTX * ctx)" 21:37:18 is different 21:37:24 jamesyonan 21:37:58 so is verify_callback OpenSSL-only now? andj 21:38:25 yes It is moved to ssl_verify_openssl.c 21:38:56 jamesyonan 21:39:55 it would be nice to have a unit test for verify_callback andj 21:42:06 It should be easier to create, as the code is slightly more isolated now jamesyonan 21:43:39 would ERR_clear_error need to be called if verify_cert causes session->verified to be set to false? andj 21:44:59 There could have been an error generating the subject oh sorry, in verify_cert 21:46:20 it is called:  err:  ERR_clear_error ();  session->verified = false; 21:46:25 https://github.com/andj/openvpn-ssl-refactoring/blob/950b2182d8846d794ca1339b8d20ad7532801c5f/ssl.c 21:46:29 vpnHelper 21:46:31 Title: ssl.c at 950b2182d8846d794ca1339b8d20ad7532801c5f from andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 21:46:33 It's just not in the diff ,as the code there doesn't chanage 21:46:42 -a 21:46:44 jamesyonan 21:52:12 well the old code always seems to call ERR_clear_error () any time session->verified is set to false andj 21:52:25 so does the new code see https://github.com/andj/openvpn-ssl-refactoring/blob/950b2182d8846d794ca1339b8d20ad7532801c5f/ssl.c#L1038 21:52:57 vpnHelper 21:52:59 Title: ssl.c at 950b2182d8846d794ca1339b8d20ad7532801c5f from andj/openvpn-ssl-refactoring - GitHub (at github.com) andj 21:53:01 nothing changed there jamesyonan 21:53:18 I get it, it's just off the end of the patch andj 21:53:25 yeah, exactly jamesyonan 21:54:49 yes, overall I think this patch is reasonable mattock 21:54:52 nice! andj is happy 21:55 andj 21:55:15 it's the basis for most of the SSL verification stuff so that's a relief 21:55:41 mattock 21:55:54 4 patches done: https://community.openvpn.net/openvpn/wiki/PolarSSLintegration vpnHelper 21:55:55 Title: PolarSSLintegration â OpenVPN Community (at community.openvpn.net) andj 21:56:03 This patch externalises subject name retrieval: https://github.com/andj/openvpn-ssl-refactoring/commit/22a6039ac3ae6b09650f74c0db65269099f829fe vpnHelper 21:56:05 Title: Commit 22a6039ac3ae6b09650f74c0db65269099f829fe to andj/openvpn-ssl-refactoring - GitHub (at github.com) jamesyonan 21:56:05 I have to leave in 5 minutes andj 21:56:09 ah, ok mattock 21:56:21 perhaps we can still cover this one ? 21:56:22 andj 21:56:50 Instead, let's talk about what we're going to do about the other patches, and set a date for the next session If that's ok? 21:56:55 mattock 21:57:01 ok jamesyonan: can you take a look at behavioral diff stuff before next session? 21:57:24 andj 21:57:29 I suggest we look around for automated tools for diffing block moves jamesyonan 21:57:57 yes, I will look into that andj 21:58:00 and I'll have a look at getting some combination of sed/grep/multiple diff calls mattock 21:58:08 sounds good next session next week, or earlier? 21:58:16 andj 21:58:19 shall we discuss this somewhere beginning of next week? say monday or tuesday? 21:58:25 mattock 21:58:36 fine with me tuesday? 21:58:47 andj 21:59:07 Tuesday is good, what time? mattock 21:59:12 same time? andj 21:59:36 OK, that's fine... But I'd prefer to only discuss the tooling then briefly and doing an actual sprint on thursday again 21:59:48 (due to lack of time) 21:59:54 mattock 21:59:55 okay, sounds good andj 22:00:07 So quick meeting tuesday, proper sprint thursday mattock 22:00:09 jamesyonan: can you attend on Tuesday at 17:00 UTC? 17:00 - 18:00 probably 22:00:15 andj 22:00:26 Later/earlier is fine with me too jamesyonan 22:00:55 22a6 looks fine andj 22:01:09 cool mattock 22:01:35 ok, let's call this a day jamesyonan 22:01:45 yes, Tuesday at 17:00 works for me andj 22:01:50 ok, thanks everyone, and speak to you on Tuesday mattock 22:02:01 we got some work done, and hopefully some tools later to ease the process in next sprint jamesyonan 22:02:02 take care guys mattock 22:02:04 bye! andj 22:02:04 and I'm really interested in what we can find tooling wise take care 22:02:07 mattock 22:02:18 I'll write the summary tomorrow, as usual andj 22:02:23 yeah, having proper automated tools would save time in the future Thanks 22:02:27 ok, I've got a solution btw, it works reasonably for this style of commits 22:06:04 git diff 365d2319d95f4374072a bbe117b0217180718f9d >../diff.txt 22:06:34 cat diff.txt |grep "^-" |sed s/^-// >removed.txt 22:07:34 at diff.txt |grep "^+" |sed s/^+// >added.txt 22:08:24 diff -u removed.txt added.txt 22:08:30 and tada 22:08:33 that works 22:09:08 mattock 22:09:16 can you give an example diff? andj 22:09:47 Want an e-mail? mattock 22:09:58 maybe pastebin, so that I can link to it in the meeting summary expire: never if ok with you 22:10:10 andj 22:10:12 Is there an openvpn one? mattock 22:10:22 hmm, there probably is andj 22:11:11 I'll just paste it, sec jamx 22:13:47 https://gist.github.com/ maybe vpnHelper 22:13:48 Title: Gist (at gist.github.com) mattock 22:14:15 jamx: nice, never seen that before andj 22:16:09 https://gist.github.com/1097957 vpnHelper 22:16:11 Title: gist: 1097957 Gist (at gist.github.com) andj 22:16:31 the diff is the second file and it clearly shows that tls_deauthenticate was added to ssl_verify.c and is now 22:16:56 *new 22:17:02 I'm actually surprised how well that low-tech solution works 22:17:31 as long as the order doesn't change 22:17:46 mattock 22:21:58 andj: very cool andj 22:27:38 #!/bin/bash git diff $1 $2 >/tmp/difftmp123.txt 22:27:38 cat /tmp/difftmp123.txt |grep "^-" |sed s/^-// >/tmp/removed123.txt 22:27:38 cat /tmp/difftmp123.txt |grep "^+" |sed s/^+// >/tmp/added123.txt 22:27:38 diff /tmp/removed123.txt /tmp/added123.txt -u 22:27:38 is a full script 22:27:42 that does it for you 22:27:46 (warning: mangles stuff in your tmp dir) 22:27:57 hmm, I reckon that should convince jamesyonan on its own 22:32:37 even if he doesn't find a cool alternative
