-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi,
Here's the summary of the previous IRC meeting. - --- COMMUNITY MEETING Place: #openvpn-devel on irc.freenode.net List-Post: openvpn-devel@lists.sourceforge.net Date: Monday 19th Jan 2015 Time: 20:00 CET (19:00 UTC) Planned meeting topics for this meeting were on this page: <https://community.openvpn.net/openvpn/wiki/Topics-2015-01-19> Next meeting has not been scheduled but will be on the same weekday and time. Your local meeting time is easy to check from services such as <http://www.timeanddate.com/worldclock> SUMMARY cron2, d12fk, jamesyonan, mattock. novaflash, plaisthos and segoon participated in this meeting. - --- Discussed the "Mac OS X Keychain management client" patch: <http://thread.gmane.org/gmane.network.openvpn.devel/9392> Some technical changes to the patch are required. Segoon will make the changes after which the patch is reviewed again. - --- Discussed the "Make OpenVPN set routes on Windows Vista and later" patch: <http://sourceforge.net/p/openvpn-gui/openvpn/ci/8195efc0d8841b52217643a43d486cda2e171fea> The changes that affect non-Windows platforms were approved (with some changes) last week. This week the Windows-specific parts (mostly interactive.c) were partially reviewed. So far no outstanding issues were found. Review will continue as soon as possible - possibly next Monday. - --- Full chatlog is attached. - -- Samuli Seppänen Community Manager OpenVPN Technologies, Inc irc freenode net: mattock -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iEYEARECAAYFAlS9cmgACgkQwp2X7RmNIqNEaQCg0NWa36I9D6YkLuWssxr27IL6 AucAoLfEyZRCIiVTyA7VnYBCoGvvy2Xh =Xb0e -----END PGP SIGNATURE-----
(20.50.37) segoon: hi, I'm the author of keychain patch (20.53.39) mattock_: hi! (20.54.30) mattock_: 6 minutes and we go live (21.01.20) mattock_: okay (21.01.38) plaisthos: I am here (21.01.48) mattock_: hi plaisthos! (21.01.55) mattock_: who else? (21.03.19) mattock: jamesyonan, d12fk, cron2? (21.03.40) jamesyonan: hi guys (21.04.47) Envil [~m...@e182061137.adsl.alicedsl.de] è entrato nella stanza. (21.04.52) d12fk: hi (21.06.12) mattock_ ha abbandonato la stanza (quit: Ping timeout: 244 seconds). (21.06.37) mattock: hi (21.06.44) mattock: let's start (21.06.55) mattock: cron2 might join us if his connectivity on the train is good enough (21.07.32) mattock: topics here: https://community.openvpn.net/openvpn/wiki/Topics-2015-01-19 (21.07.34) vpnHelper: Title: Topics-2015-01-19 – OpenVPN Community (at community.openvpn.net) (21.07.49) mattock: first topic is segoon's "Mac OS X Keychain management client" (21.07.56) mattock: second version of the keychain patch (21.08.15) mattock: we simply did not have time to review this last week, so now it's topic #1 (21.13.28) mattock: plaisthos, jamesyonan: any thoughts? (21.13.46) mattock: http://thread.gmane.org/gmane.network.openvpn.devel/9392 (21.13.49) vpnHelper: Title: Gmane Loom (at thread.gmane.org) (21.14.41) jamesyonan: segoon: so does this iteration of the patch query the management interface to do the RSA signing? (21.15.34) segoon: jamesyonan: right (21.22.28) mattock: jamesyonan: how does that patch look? (21.22.49) jamesyonan: looking at it now -- seems okay (21.23.13) mattock: ACKable? (21.23.22) jamesyonan: so you've added management-external-cert directive as well (21.23.42) segoon: yes (21.24.00) segoon: it would look better if it looked like rsa-sign (21.24.13) segoon: (multiline argument) (21.24.31) jamesyonan: how is the client certificate actually selected from the keychain? (21.24.43) segoon: but I hesitated to add specific cmd for that (21.24.56) segoon: absolutely the same way as in the first revision of the patch (21.25.45) jamesyonan: so the config file has to identity the cert? (21.25.57) segoon: look at findIdentity(): SecItemCopyMatching()+SecIdentityCopyCertificate() (21.26.19) segoon: what do you mean by 'config file'? (21.26.38) jamesyonan: openvpn config file (21.27.04) AnCron [~gert@openvpn/community/developer/cron2] è entrato nella stanza. (21.27.04) modalità (+o AnCron) da ChanServ (21.27.24) segoon: I've implemented keychain stuff as an external daemon, cert template is passed as a cmdline arg (21.27.29) mattock: AnCron: welcome :) (21.27.50) segoon: if you run it automatically when openvpn is started then yes, config contains this cert template string (21.28.12) AnCron: Whee, this works :) (21.28.16) mattock: yep (21.28.41) mattock: we've been discussing the MacOS X keychain patch: (21.28.41) mattock: http://thread.gmane.org/gmane.network.openvpn.devel/9392 (21.28.43) vpnHelper: Title: Gmane Loom (at thread.gmane.org) (21.32.11) jamesyonan: So when keychain-mcd is doing the SecItemCopyMatching to find the identity, what criteria is it using, i.e. how is the identity configured? (21.35.05) segoon: user passes an identity template, like 'subject:c=ru' (21.35.25) segoon: findIdentity() iterates over all identities and tries to match this template with current identity (21.35.59) segoon: if >0 identities are matched, then the freshest identity is chosed (21.36.02) segoon: *chosen (21.36.43) segoon: if no identity matches the template, it is an error, obviously (21.36.50) jamesyonan: For example, if I'm implementing a UI, I might want to show the user a list of possible identities and allow them to choose one (21.37.35) jamesyonan: that UI would then need to interact with keychain-mcd in some way to do this? (21.37.50) segoon: I think UI can iterate over all identities itself and choose any identity, save its SHA hash and pass it to keychain-mcd as a template (21.38.22) segoon: MD5 or SHA1 hash (21.38.48) segoon: via 'SHA1: 30 F7 3A ...' or "MD5: D5 F5 11...' template (21.38.55) jamesyonan: so UI could choose a specific identity from keychain and then pass it to keychain-mcd using its hash? (21.39.39) segoon: right, it is possible to choose a specific identity by its hash, serial number or any other unique property (21.40.31) segoon: or, alternatively, user is able to choose an identity by subject:cn= and issuer:*anything* (21.42.22) AnCron ha abbandonato la stanza (quit: Ping timeout: 240 seconds). (21.43.08) jamesyonan: a couple more points… looking at cert_to_pem function. I'm not very comfortable with too much arithmetic in the size argument of malloc that, if wrong, might generate a buffer overflow later on. (21.43.53) jamesyonan: can you use some kind of wrapper when constructing the string that guarantees that no buffer overflow can occur, such as by using struct buffer? (21.45.09) jamesyonan: I think that's better than writing to a buffer without constraint, on the assumption that the size parameter passed to malloc is exactly correct (21.45.10) segoon: yes, it would look much better (21.47.27) jamesyonan: another thing: you've bumped up USER_PASS_LEN from 128 to 12800 -- that's quite a large increase. (21.47.52) segoon: right, that's one thing that I'm not proud of (21.48.05) segoon: I'd prefer a kind of dynamic buffer (21.48.42) jamesyonan: agreed -- dynamic buffer would be better (21.49.10) jamesyonan: why did you need to increase it so much? (21.49.44) segoon: or -- moving from needok cmd to something else that doesn't use struct user_pass (21.50.14) segoon: 12800 is an arbitrary number, but the old limit was too low for certificate passing (21.50.46) segoon: certificate is passed as base64-encoded string, in a second argument of needok (21.51.03) segoon: needok cert big-string (21.51.05) jamesyonan: you also increased OPTION_PARM_SIZE quite a bit (21.51.17) segoon: right, I had to increase several limits (21.51.50) jamesyonan: all of these will negatively affect memory consumption even in cases where your code is not used (21.52.50) segoon: I tried to find anything with no hardcoded data limit like rsa-sign, but failed (21.53.09) segoon: rsa-sign is quite nonextendable to other usages (21.53.42) segoon: rsa-sign's format is ideal for me -- the same big base64 string divided into multiple lines (21.57.25) jamesyonan: so you are actually transmitting full certs via user/pass struct? (21.57.55) segoon: yes (21.58.27) jamesyonan: why not just transmit the fingerprint? (21.58.47) segoon: for what? (21.59.03) segoon: to make it possible for openvpn to obtain the cert from keychain? (22.00.02) segoon: the thing is that keychain query is 80% of the code (22.00.18) jamesyonan: I think I see -- you need the whole cert for managment-query-cert not just for cert selection? (22.00.46) jamesyonan: there are existing pathways in the management interface for dealing with multi-line queries (22.02.05) jamesyonan: for example client-auth and client-pf are multi-line queries (22.02.56) ender` [krneki@2a01:260:4094:1:42:42:42:42] è entrato nella stanza. (22.03.17) jamesyonan: it just seems wrong to expand USER_PASS_LEN by 100x because you want to pass new data types that are not really user/pass data. (22.04.12) segoon: agreed, I looked at Android stuff, it uses user_pass but the param length was very short (22.04.16) segoon: looking into client-auth.. (22.04.53) ender| ha abbandonato la stanza (quit: Ping timeout: 272 seconds). (22.09.28) segoon: I don't fully understand how params are passed, simply via p[]? (22.09.45) segoon: up to 'END' line, right? (22.11.03) segoon: no, I'm wrong (22.13.42) segoon: ah, callback is called for each line until man->connection.in_extra == NULL (22.14.06) jamesyonan: see #ifdef MANAGEMENT_IN_EXTRA stuff (22.16.04) segoon: yes, it is much simplier than current approach (22.16.27) segoon: but it need another command type (22.17.41) segoon: like 'NEED-CERT' as a server request and 'certificate' as client answer (22.17.55) jamesyonan: I think it would be cleaner to add another command type because passing certs in the managment interface isn't something we've done before (22.18.18) jamesyonan: agreed, NEED-CERT seems like a better approach (22.18.27) segoon: I like the idea (22.19.16) segoon: in this case no limits (line, param, pass, etc.) should be increased (22.19.42) segoon: I'll fix this in the next patch version (22.19.58) jamesyonan: okay, sounds good (22.20.22) segoon: any other questions to discuss? (22.21.00) jamesyonan: no, I think we can move on (22.21.10) mattock: ok :) (22.21.12) mattock: good discussion (22.21.22) mattock: d12fk: still there? (22.21.29) segoon: okay, thank you for the critique (22.21.30) d12fk: jup (22.21.39) mattock: segoon: good you could make it! (22.21.48) mattock: http://thread.gmane.org/gmane.network.openvpn.devel/9237 (22.21.50) vpnHelper: Title: Gmane Loom (at thread.gmane.org) (22.21.55) cron2: +1 (22.22.21) mattock: jamesyonan: as I mentioned, d12fk's patch was reviewed except for the Windows-specific bits (22.22.22) segoon: good night everybody (22.22.27) mattock: good night! (22.22.31) d12fk: night (22.22.53) mattock: so now we'd need to have a look at the Windows part (22.23.30) segoon ha abbandonato la stanza (quit: Quit: WeeChat 0.3.7). (22.24.43) d12fk: jamesyonan: mostly what's happening in interactive.c (22.26.04) jamesyonan: do you have a link to the latest patch? (22.27.51) d12fk: do you want the code or a patch (patch is huge) (22.28.12) jamesyonan: patch is fine (22.28.28) d12fk: http://sourceforge.net/p/openvpn-gui/openvpn/ci/8195efc0d8841b52217643a43d486cda2e171fea/ (22.30.50) jamesyonan: how do you ensure that the pipe can't be hijacked by another process? (22.31.46) d12fk: do you mean the pipe between the service and openvpn? (22.31.56) jamesyonan: yeah (22.32.41) d12fk: let me check, it has been a while (22.34.12) cron2: iirc user access rights (22.35.57) novaflash: NO WAY (22.36.06) novaflash: this was totally a coincidence (22.36.20) novaflash: but i'm glad to see the admin rights issue is on the table :-D (22.36.46) jamesyonan: for example, could a rogue process impersonate openvpn.exe and get the service to do stuff that would normally not be allowed for non-admin processes to do, like add a default route to the routing table? (22.37.07) mattock: novaflash is referring to a feature request he just made to me, without knowing anything about d12fk's patch :) (22.37.15) novaflash: amazing coincidence (22.37.28) d12fk: the handle for the pipe is inherited by the child process (openvpn) and ovpn duplicates it so it so it can use it. other processes cannot duplicate the handle because they cannot access openvpn's memory (22.38.19) d12fk: the trick is that ovpn is run as the user that requested it to run, but the process owner is the service (22.38.19) cron2: anonymous pipe (22.38.53) d12fk: so no one but the system user can modify the DACL (22.38.56) jamesyonan: so the service actually spawns openvpn.exe but impersonating the creds of the UI client? (22.39.12) d12fk: yes (22.39.52) jamesyonan: do you check that openvpn.exe cannot have been moved or modified by non-admin user? (22.40.59) d12fk: no currently i trust that ProgramFiles\* is not modifyable by users (it is by default) (22.42.02) mattock: d12fk: or "Program Files (x86)" I presume? (22.42.21) d12fk: mattock: depends on how you built it =P (22.43.05) cron2: ok, i'm off again... bus arriving, go find hotel (22.43.19) d12fk: bye (22.43.23) mattock: cron2: bye! (22.43.59) novaflash: for a guy with the nickname for a scheduling program, he has bad timing (22.44.22) cron2: :-P (22.48.47) mattock: jamesyonan: reviewing the patch? (22.49.00) mattock: reading I mean (22.49.28) jamesyonan: I assume you're aware of the popular CreateProcess vulnerability where if you forget to quote the exe path, you can end up executing c:\Program.exe ? (22.49.42) jamesyonan: yes, so far the patch looks fine (22.49.49) mattock: great! (22.50.21) mattock: I will have to check out in 10 minutes, but my presence is hardly necessary (22.52.13) d12fk: jamesyonan: not sur what you are refering to but i've read the man page back then and made sure all the pitfalls do not apply (22.52.33) jamesyonan: I've gotta run too -- so far patch looks good, though. (22.52.33) d12fk: *msdn page (22.52.50) jamesyonan: what are the specific changes you wanted me to review wrt. windows routing? (22.52.54) d12fk: yes keep on reviewin in silent moments and we talk again another monday (22.53.05) mattock: next monday I hope (22.53.12) mattock: so that we don't forget the details (22.53.32) novaflash: d12fk: basically, if you do something like: start c:\program files (x86)\openvpn client\bin\openvpngui.exe, and you have a file called program.exe in c:\, then it will run that instead of assuming it should go to the folder and run the program in that folder. that's the vulnerability. the solution is to enclose with quotes. (22.53.56) jamesyonan: CreateProcess has a well-known vulnerability if you don't quote exe names that have spaces, as most Program Files exe names do (22.54.33) jamesyonan: ok, sound good (22.54.39) d12fk: but you have to quote c strings anyway. dont get it... (22.54.51) jamesyonan: no, quote inside a quote (22.55.20) jamesyonan: If I called CreateProcess with c:\program files\fooapp (22.55.22) novaflash: d12fk: i was oversimplying things but createprocess has a bug/vulnerability in it that leaves out the quotes you put around it, treating your quoted input as unquoted (22.55.50) jamesyonan: then c:\program.exe would be executed if it exists (22.56.08) d12fk: was that appying to the unicode version of it? (22.56.15) jamesyonan: not sure (22.56.18) novaflash: iunno (22.57.07) d12fk: it's years ago i wrote that code. i'll check the docs again (22.59.56) d12fk: ah it only appies if the first param id NULL and you have the path to the binary in the second arg (23.00.02) d12fk: *applies (23.00.32) d12fk: The lpApplicationName parameter can be NULL. In that case, the module name must be the first white space–delimited token in the lpCommandLine string. If you are using a long file name that contains a space, use quoted strings to indicate where the file name ends and the arguments begin; otherwise, the file name is ambiguous. (23.00.59) d12fk: not doing anything that dirty (23.03.31) krzee: o/ (23.04.11) mattock: ok guys, let's try to resume the review next month (23.04.18) d12fk: k (23.04.26) mattock: sorry, next _week_ :P (23.04.33) mattock: we will eventually get this merged
openvpn_irc_meeting_chatlog_2015-01-19.txt.sig
Description: PGP signature
