Open VPN client support: https://openvpn.net/index.php/open-source/documentation/security-overview.html The idea here is to implement a compatible client - not to import non MPL compatible code. This would be primarily for use on Firefox OS and would require operating system support. On FFOS that would be TUN/TAP - but any IDL interfaces should be OS neutral even if they are not implemented at this time. We'll need both UDP and TCP modes. (UDP being the better mapping for tunneling packets that are already subject to congestion control, and TCP as it maybe necessary to traverse some firewalls). There should be an xpcom accessible (chrome permissions) interface to create and destroy vpns. There should be an interface to route to particular destination ip addresses over the vpn. There should be an interface to route particular loadgroups over the vpn. There should be an interface to route particular appids over the vpn. There should be statistics on the vpn for bytes sent and received and maybe timestamp of formation. eventually there will need to be some UI settings to get at those interfaces. We probably need other stuff too :)

For the openvpn stuff i will need a way to calculate HMAC for each packet sent and received. What is the best way to do this? Using nsICryptoHMAC?

Yes, nsICryptoHMAC should work for that. See also https://mxr.mozilla.org/mozilla-central/source/services/crypto/modules/utils.js for some utilities for working with nsICryptoHMAC (if you're implementing this in js, that is).

(In reply to David Keeler (:keeler) [use needinfo?] from comment #2) Thanks.

This are changes, I needed in nss. Can you look at them. I will probably extend KeyModule to add some other key types. In nsStreamCipher I added function InitWithIVAndOperation because I did not want to change old functions (actually internally I did change them). Probably I can merge them into one function. I have fixed nsStreamCipher to work with padded encryptions. If it is not possible to change nsStreamCipher, I will make own interface. I also added nsITLSHelpFunctions with one function that I needed. If you have suggestion for a better name I will be happy to change it. Thanks.

Comment on attachment 8486532 [details] [diff] [review] Security part fix V1 Review of attachment 8486532 [details] [diff] [review]: ----------------------------------------------------------------- In general this looks good. I have some style nits and other comments that should probably be addressed. ::: security/manager/ssl/public/nsIKeyModule.idl @@ +15,5 @@ > > // Algorithm types > const short RC4 = 1; > const short AES_CBC = 2; > + const short AES_CBC_PAD = 3; Hmmm - does AES_CBC not do padding? That would probably be a bad thing. @@ +16,5 @@ > // Algorithm types > const short RC4 = 1; > const short AES_CBC = 2; > + const short AES_CBC_PAD = 3; > + const short BLOWFISH_CBC = 4; Similarly, does this do padding? ::: security/manager/ssl/public/nsIStreamCipher.idl @@ +35,5 @@ > + * @param aKey nsIKeyObject > + * @param aIV the initialization vector > + * @param aIVLen the length of the initialization vector > + */ > + void initWithIVAndOperation(in nsIKeyObject aKey, Honestly, I think it would be best to just update initWithIV (and maybe the other methods) to take a "mode" enumeration that's basically one of { Encrypt, Decrypt } and maybe something else. ::: security/manager/ssl/public/nsITLSHelpFunctions.idl @@ +8,5 @@ > + > +[scriptable, uuid(a9f7b27d-2256-47fa-ba28-abf82b41e69f)] > +interface nsITLSHelpFunctions : nsISupports { > + > + void PerformTlsPRF([array, size_is(aSecretLen)] in octet aSecret, in unsigned long aSecretLen, In what context will this be used? Can this be a mode of nsIStreamCipher? Or can window.crypto.getRandomValues be used? ::: security/manager/ssl/src/nsStreamCipher.cpp @@ +90,5 @@ > + return InitWithIV_(aKey, &IV, true); > +} > + > +NS_IMETHODIMP nsStreamCipher::InitWithIVAndOperation(nsIKeyObject *aKey, > + const uint8_t *aIV, nit: 'const uint8_t* aIV', etc. @@ +166,5 @@ > { > if (!mContext) > return NS_ERROR_NOT_INITIALIZED; > > + if (mMaxPadSize) { if mMaxPadSize is 0, is it an error? Should we report that to the user? ::: security/manager/ssl/src/nsTLSHelpFunctions.cpp @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- > + * nit: no empty line here @@ +3,5 @@ > + * This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + nit: only one blank line here @@ +34,5 @@ > + > + if (!symKey) { > + return NS_ERROR_FAILURE; > + } > + SECItem param = {siBuffer, NULL, 0}; What owns the memory that I'm assuming gets put in param? Do we need to free it? (Note: if so, use SECITEM_FreeItem(&param, false)) @@ +51,5 @@ > + rv = MapSECStatus(PK11_DigestOp(prf_context.get(), aLabel, aLabelLen)); > + if (NS_FAILED(rv)) { > + return rv; > + } > + rv = MapSECStatus(PK11_DigestOp(prf_context.get(), aSeed, aSeedLen)); nit: spaces, not tabs

thanks for looking at this > > ::: security/manager/ssl/public/nsIKeyModule.idl > @@ +15,5 @@ > > > > // Algorithm types > > const short RC4 = 1; > > const short AES_CBC = 2; > > + const short AES_CBC_PAD = 3; > > Hmmm - does AES_CBC not do padding? That would probably be a bad thing. > if AES_CBC data are decrypted and function PK11_CipherOp returns all decrypted data including padded bytes. Therefore I start using AES_CBC_PAD. > @@ +16,5 @@ > > // Algorithm types > > const short RC4 = 1; > > const short AES_CBC = 2; > > + const short AES_CBC_PAD = 3; > > + const short BLOWFISH_CBC = 4; > > Similarly, does this do padding? > I have not tried this i wanted to but i did not have time. Anyway, in next week(s) we will need to decide which mechanisms we will support and only these mechanisms will be available and tested. > ::: security/manager/ssl/public/nsIStreamCipher.idl > @@ +35,5 @@ > > + * @param aKey nsIKeyObject > > + * @param aIV the initialization vector > > + * @param aIVLen the length of the initialization vector > > + */ > > + void initWithIVAndOperation(in nsIKeyObject aKey, > > Honestly, I think it would be best to just update initWithIV (and maybe the > other methods) to take a "mode" enumeration that's basically one of { > Encrypt, Decrypt } and maybe something else. > I was thinking the same. It looked strange that only encrypt is supported, but i did not know why so i have not touched initWithIV. I will update this function and init(...) > ::: security/manager/ssl/public/nsITLSHelpFunctions.idl > @@ +8,5 @@ > > + > > +[scriptable, uuid(a9f7b27d-2256-47fa-ba28-abf82b41e69f)] > > +interface nsITLSHelpFunctions : nsISupports { > > + > > + void PerformTlsPRF([array, size_is(aSecretLen)] in octet aSecret, in unsigned long aSecretLen, > > In what context will this be used? Can this be a mode of nsIStreamCipher? Or > can window.crypto.getRandomValues be used? > OpenVPN use this function to generate keys so the same function must be use in our client as on the server. So it needs to have secret, label and seed as parameter. We can put this function into nsIRandomGenerator. What do you think? > > @@ +166,5 @@ > > { > > if (!mContext) > > return NS_ERROR_NOT_INITIALIZED; > > > > + if (mMaxPadSize) { > > if mMaxPadSize is 0, is it an error? Should we report that to the user? > this is an internal error, so it should not happened at all. I will change so that it crashes.

An update of security part.

OpenVPN for review... I hope I do not have to change a lot :)

Here is a new version, if you havn't already started with the review... I added IOService part and a new state "Offline" and fix some small things.

Comment on attachment 8510294 [details] [diff] [review] openVPN client v2 Review of attachment 8510294 [details] [diff] [review]: ----------------------------------------------------------------- this is an impressive submission. thanks! I'll admit I didn't do a deep dive on all of it - but I have read it all and think I've identified enough basic things that we should resolve them before iterating again. are all those idls things that need to be exposed? typically it would be in an idl if we wanted chrome to be able to use it or something significantly out of module.. I had concerns about some things being exposed in them that I wouldn't blink at if they were just h files. (routing table ids, for example). how do we test this.. an example with a pac would be excellent (even if at first its manual) ::: netwerk/base/public/nsIProtocolProxyService2.idl @@ +35,5 @@ > */ > nsICancelable asyncResolve2(in nsIURI aURI, in unsigned long aFlags, > in nsIProtocolProxyCallback aCallback); > + > + nsICancelable asyncResolveWithAppId(in nsIURI aURI, in unsigned long aAppId, some documentation is needed.. also bump the uuid ::: netwerk/base/src/ProxyAutoConfig.cpp @@ +465,5 @@ > +{ > + JS::CallArgs args = JS::CallArgsFromVp(argc, vp); > + > + if (NS_IsMainThread()) { > + NS_WARNING("PACMyAppId on Main Thread. How did that happen?"); MOZ_ASSERT(false) @@ +944,5 @@ > +bool > +ProxyAutoConfig::MyAppId(const JS::CallArgs &aArgs) > +{ > + nsAutoCString remoteDottedDecimal; > + nsAutoCString localDottedDecimal; unused vars ::: netwerk/base/src/nsPACMan.h @@ +109,5 @@ > * @param mustCallbackOnMainThread > * If set to false the callback can be made from the PAC thread > */ > + nsresult AsyncGetProxyForURI(nsIURI *uri, uint32_t appId, > + nsPACManCallback *callback, appid needs to be be added to the comment above ::: netwerk/base/src/nsProtocolProxyService.cpp @@ +38,5 @@ > extern const char kProxyType_HTTPS[]; > extern const char kProxyType_SOCKS[]; > extern const char kProxyType_SOCKS4[]; > extern const char kProxyType_SOCKS5[]; > + extern const char kProxyType_VPN[]; these are all documented in an idl somewhere.. the new string needs to be added ::: netwerk/base/src/nsProtocolProxyService.h @@ +204,5 @@ > * @param result > * The resulting proxy info or null. > */ > nsresult Resolve_Internal(nsIURI *uri, > + uint32_t appId, comments above need to reflect new param ::: netwerk/base/src/nsSocketTransport2.cpp @@ +806,3 @@ > } > > const char *proxyType = nullptr; it looks like this declaration can be moved up before the above condition and then the two conditionals can be combined.. @@ +1368,5 @@ > if (status != PR_SUCCESS) { > return NS_ERROR_FAILURE; > } > mBindAddr = nullptr; > + } else if (fd && mUseVPN) { I'm trying to figure out why this is the else() to the if(mBindAddr).. ,that would seem surprising if you have the vpn.. what if we go down the vpn route first, and when the vpn code tries to do a bind if there is also an mBindAddr that also doesn't match we throw an err? @@ +1369,5 @@ > return NS_ERROR_FAILURE; > } > mBindAddr = nullptr; > + } else if (fd && mUseVPN) { > + nsresult rv; no need to redeclare rv @@ +1372,5 @@ > + } else if (fd && mUseVPN) { > + nsresult rv; > + nsCOMPtr<nsIOpenVPNProvider> vpnPrv = > + do_GetService(NS_OPENVPNPROVIDER_CONTRACTID, &rv); > + if (NS_SUCCEEDED(rv)) { what if it fails? We need to throw an error I think.. that seems true for all the conditionals in this block. ::: netwerk/openvpn/OpenVPN.cpp @@ +27,5 @@ > +namespace net { > + > +// The length of queues that hold packets between openVPNSocket > +// and TunInterface. > +#define OPENVPN_MAX_QUEUE_LENGTH 2 this just seems really small. maybe I misunderstand its role. (and my comment in the h file about the queue implementation is related.. obviously if the q is only 2 deep it doesn't matter) @@ +54,5 @@ > + WaitOpenVPNToCleanUp(nsIOpenVPN *aOVPN) > + : mOVPN(aOVPN) > + {} > + > + NS_IMETHOD Run() the whole point of this is to do the release on the mainthread? you can do that with nsMainThreadPtrHolder<nsIOpenVPN> much easier and better. @@ +94,5 @@ > +} > + > +OpenVPN::~OpenVPN() > +{ > + MOZ_ASSERT(mState == nsIOpenVPN::DELETED); should it just call delete() itself if not deleted? We can't really assert on proper use of an idl.. callers will be dumb. @@ +122,5 @@ > + if (PR_GetCurrentThread() != gSocketThread) { > + mTarget->Dispatch(NS_NewRunnableMethod(this, > + &nsIOpenVPN::Start), > + NS_DISPATCH_NORMAL); > + return NS_OK; so I'm worried about start() and a race condition with offline (or deleted). main thread calls start.. its online and the new runnable-start gets dispatched to the socket thread, and them main thread gets ok. now we go offline now runnable-start executes, but it returns NS_ERROR_NOT_AVAILABLE because we're offline. but the caller of Start() can't know this as its no longer on the stack. @@ +133,5 @@ > + // First start OpenVPNSocket before starting tun. In most cases informations > + // from server are needed before tun can be configured. > + nsresult rv = mOpenVpnSocket->Init(); > + if (NS_FAILED(rv)) { > + NS_WARNING(nsPrintfCString("OpenVPN::Start [this=%p]: OpenVPNSocket " make sure this goes into the nspr log too @@ +188,5 @@ > + > + while (mFromTunToPeer.Length()) { > + struct PacketBuffer* pct = mFromTunToPeer.LastElement(); > + mFromTunToPeer.RemoveElementAt(mFromTunToPeer.Length() - 1); > + free(pct); The queues would be better defined (if we keep them as arrays) as nsTArray<nsAutoPtr<PacketBuffer> > .. then normal c++ dtors will prevent leaks and we can get rid of the manual frees. it would just be mFromTunToPeer.Clear() if we needed to clear them outside of ~openvpn() @@ +261,5 @@ > + } > + if (mState == nsIOpenVPN::STOP || IsOffline()) { > + // It is already not running. Just dispatch WaitOpenVPNToCleanUp to be sure. > + ChangeState(nsIOpenVPN::DELETED); > + WaitOpenVPNToCleanUp *event = new WaitOpenVPNToCleanUp(this); nsrefPtr<WaitOpenVPNToCleanup> event = new.. @@ +265,5 @@ > + WaitOpenVPNToCleanUp *event = new WaitOpenVPNToCleanUp(this); > + nsresult rv = mTarget->Dispatch(event, NS_DISPATCH_NORMAL); > + if (NS_FAILED(rv)) { > + // Probably socket thread is shutdown. > + free(event); you don't need the free with the refptr but you probably want to return an err from the method @@ +276,5 @@ > + rv = mTarget->Dispatch(NS_NewRunnableMethod(this, > + &OpenVPN::DeleteInternal), > + NS_DISPATCH_NORMAL); > + if (NS_FAILED(rv)) { > + DeleteInternal(); return code here? @@ +287,5 @@ > +void > +OpenVPN::DeleteInternal() > +{ > + StopInternal(false); > + WaitOpenVPNToCleanUp *event = new WaitOpenVPNToCleanUp(this); refptr again @@ +302,5 @@ > + if (mState != nsIOpenVPN::RUNNING) { > + return NS_ERROR_NOT_AVAILABLE; > + } > + if (!mTun) { > + aTunName = ""; aTunName.Assign(EmptyCString()) @@ +365,5 @@ > + > + return mTun->SetParameters(aTunVPNAddr, aRemotePeerVPNAddr); > +} > + > +// Not implemented. why not? (why have it if not implemented..) @@ +612,5 @@ > +} > + > +NS_IMETHODIMP > +OpenVPN::NotifyObservers(uint32_t aState) > +{ enforce the main thread @@ +732,5 @@ > + "RUNNING", this)); > + mRetriedConnections = 0; > + if (mState == nsIOpenVPN::INITIALIZING) { > + // Get addresses and start tun. > + nsCString localAddr; you can make these nsAutoCString on the stack @@ +740,5 @@ > + mTun->SetCallBack(this); > + nsresult rv = mTun->Init(); > + if (NS_FAILED(rv)) { > + ChangeState(nsIOpenVPN::ERROR); > + StopInternal(true); shoul this return? @@ +791,5 @@ > + mTimeActiveUntilLastStart += (now - mLastStart); > + } > + mState = aNewState; > + > + VPNStateChanged *event = new VPNStateChanged(this, mState); nsrefptr<vpnstatechanged> event = new.. @@ +792,5 @@ > + } > + mState = aNewState; > + > + VPNStateChanged *event = new VPNStateChanged(this, mState); > + nsresult rv = NS_DispatchToMainThread(event); this just becomes return NS_Dispatch.. ::: netwerk/openvpn/OpenVPN.h @@ +41,5 @@ > + void ChangeState(uint32_t aNewState); > + void ChangeStateWithLock(uint32_t aNewState); > + bool IsOffline(); > + friend class VPNStateChanged; > + struct PacketBuffer { class @@ +42,5 @@ > + void ChangeStateWithLock(uint32_t aNewState); > + bool IsOffline(); > + friend class VPNStateChanged; > + struct PacketBuffer { > + char *mBuf; you can use nsAutoArrayPtr<char> here to have the dtor do the cleanup c++ style @@ +71,5 @@ > + > + Mutex mAccessLock; > + uint32_t mState; > + > + RoutingRules routingRules; mRoutingRules @@ +78,5 @@ > + nsRefPtr<OpenVPNSocket> mOpenVpnSocket; > + int32_t mMaxConnectionRetries; > + int32_t mRetriedConnections; > + > + nsTArray<struct PacketBuffer*> mFromTunToPeer; so these are queues, right? If feels like a nstarray is the wrong thing to use.. I don't think it can cheaply delete from 0 without shifting the array. I'm sure there is a linked list class floating around somewhere which will have O(1) append and delete.. maybe I'm wrong and nstarray is doing a smart indirection under the covers. worth checking. @@ +85,5 @@ > + bool mPacketWaitingFromTun; > + uint32_t mMtuToTun; > + nsCOMPtr<nsIEventTarget> mTarget; > + > + PRIntervalTime mLastStart; there are some real range limitations on printervatime and its hard to use correctly. use mozilla::timestamp instead. ::: netwerk/openvpn/OpenVPNProvider.cpp @@ +25,5 @@ > +// OpenVPN > +//------------------------------------------------------------------------------ > + > +OpenVPNProvider::OpenVPNProvider() > + : mLock("OpenVPNProvider access lock") so we've got a lock which implies cross thread access. can you explain what methods are expected to be called from what threads? @@ +49,5 @@ > + MutexAutoLock lock(mLock); > + int inx = 0; > + for (; inx < MAXROUTINGTABLE; inx++) { > + if (routingTable[inx] == 0) { > + routingTable[inx] = 1; so it seems like routingTable[] is an array of bool, not int.. @@ +56,5 @@ > + } > + if (inx == MAXROUTINGTABLE) { > + LOG(("OpenVPNProvider::CreateVPNClient: Too many openVPN clients " > + "configured")); > + return NS_ERROR_FAILURE; NS_ERROR_OUT_OF_MEMORY @@ +62,5 @@ > + > + nsRefPtr<OpenVPN> vpn = new OpenVPN(); > + > + vpn->SetRoutingTableId(252 - inx); // 253 is default table. Table index > + // between 252 and 252. you mean 252 - MAXROUTINGTABLE.. isn't this really fragile and depends on local config state? @@ +74,5 @@ > +NS_IMETHODIMP > +OpenVPNProvider::Shutdown() > +{ > + MutexAutoLock lock(mLock); > + for (uint32_t i =0; i < mVPNs.Length(); i++) { routingtable[i] = 0 ? @@ +123,5 @@ > +OpenVPNProvider::GetTunNameForLocalVPNAddr(const nsACString &aLocalAddr, > + nsACString &aTunName) > +{ > + MutexAutoLock lock(mLock); > +/* why is this not working? @@ +142,5 @@ > +OpenVPNProvider::GetTunNameForRemotePeerAddr(const nsACString &aPeerAddr, > + nsACString &aTunName) > +{ > + MutexAutoLock lock(mLock); > + for (uint32_t i = 0; i < mVPNs.Length(); i++) { you probably should have an inlined helper function "nsIOpenVPN *FindOpenVPNByPeerAddr (const nsACString &aPeerAddr)" ::: netwerk/openvpn/OpenVPNProvider.h @@ +21,5 @@ > + > +// The maximal number of routing table ids. This will be the maximal number of > +// clients that can be configured at a time. TODO discuss probably 10 is too > +// much? > +#define MAXROUTINGTABLE 10 why do we need a max? @@ +36,5 @@ > + ~OpenVPNProvider(); > + > + Mutex mLock; > + nsCOMArray<nsIOpenVPN> mVPNs; > + uint8_t routingTable[MAXROUTINGTABLE]; mRoutingTable ::: netwerk/openvpn/OpenVPNSocket.cpp @@ +45,5 @@ > +//------------------------------------------------------------------------------ > + > +OpenVPNSocket::PacketBuffer::PacketBuffer() > +{ > + mBuf = nullptr; you can manage mBuf with nsAutoArrayPtr @@ +103,5 @@ > +} > + > +OpenVPNSocket::TlsSession::~TlsSession() > +{ > + free(mPreMasterKey); again c++ management here.. @@ +136,5 @@ > +//------------------------------------------------------------------------------ > + > +OpenVPNSocket::OpenVPNSocket() > + : mRemotePeerPort(1194) > + , mProtocol("udp") NS_LITERAL_CSTRING("udp") @@ +208,5 @@ > + > +NS_IMETHODIMP > +OpenVPNSocket::Init() > +{ > + MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); so you have an idl here that can only be called on the socket thread. That's a bit unusual.. perhaps it shouldn't be an idl? (i.e. what other module will want to call this? js can't do it because it needs to be on the socket thread) @@ +218,5 @@ > + } > + > + mTarget = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv); > + if (NS_FAILED(rv)) { > + NS_WARNING(nsPrintfCString("OpenVPNSocket::Init [this=%p]: No transport " nspr log wherever you have warnings (the warnings are nice, but the nspr log is what will diagnose field problems) @@ +322,5 @@ > + > + mTlsSessionId = 0; > + mTlsRemoteSessionId = 0; > + > + while (mTlsSubSession.Length()) { all of these queues (mtlssubsession, msentpackets, mwritequeue) can just become queue.Clear() if they hold smart ptrs. @@ +433,5 @@ > + : mOpenVPNSocket(aOpenVPNSocket) > + , mState(aState) > + {} > + > + NS_IMETHOD Run() assert the thread you expect run to be on.. useful for documentation if nothing else @@ +454,5 @@ > + mState = aNewState; > + // TODO: if it is an error send a also a error type (net error, tls error...). > + // It would be useful for UI. > + if (mStateCallBack) { > + VPNSocketStateChanged *event = new VPNSocketStateChanged(this, mState); nsrefptr @@ +487,5 @@ > + > + // Wait for response. > + nsresult rv = mStreamIn->AsyncWait(this, 0, 0, mTarget); > + if (NS_FAILED(rv)) { > + return NS_ERROR_FAILURE; return rv? @@ +500,5 @@ > + rv = mTlsHandshakeTimer->InitWithCallback(this, > + PR_IntervalToMilliseconds(mTlsHandshakeTimeout), > + nsITimer::TYPE_ONE_SHOT); > + if (NS_FAILED(rv)) { > + return NS_ERROR_FAILURE; return rv? @@ +904,5 @@ > + > +NS_IMETHODIMP > +OpenVPNSocket::GetTunMtu(uint32_t *aTunMtu) > +{ > + MOZ_ASSERT(aTunMtu); if you keep this as an IDL you want to make that NS_ENSURE_ARG_POINTER.. we can't assert that some other module follows the calling pattern, we can only do that internal to necko code. (the former will return an err if its null).. this applies to a bunch of methods here. otoh if the xpcom idl goes away you can and should assert that the rest of necko is being sane. @@ +1019,5 @@ > + if (NS_FAILED(rv)) { > + NS_WARNING(nsPrintfCString("OpenVPNSocket::CreateNewSession [this=%p]: " > + "Failed creating the random number generator, " > + "use rand.", this).get()); > + *sessionId = ((uint64_t)rand() << 32) + (uint64_t)rand(); I'm going to say we should refuse to create the session if the trusted rng isn't available. @@ +1031,5 @@ > + this).get()); > + *sessionId = ((uint64_t)rand() << 32) + (uint64_t)rand(); > + } > + } > + mTlsSessionId = *sessionId; why this whole dance with the ptr and the malloc.. why not just read the randombytes into &mTlsSesisonId @@ +1053,5 @@ > + "new keyId is %d.", this, key, mTlsSessionId)); > +} > + > +void > +OpenVPNSocket::PrepareOptionString() some kind of documentation here @@ +1165,5 @@ > + return NS_OK; > +} > + > +nsCString > +OpenVPNSocket::CipherToString() use NS_LITERAL_CSTRING in here @@ +1216,5 @@ > + return rv; > +} > + > +void > +OpenVPNSocket::SetRandomKeys(struct OpenVPNSocket::TlsSession *aSession) any failures here ought to be really a vpn failure, right? @@ +1249,5 @@ > + rnd = rand(); > + memcpy(aSession->mKey2 + i * 4, &rnd, 4); > + } > + } > + } else { else fail! @@ +1294,5 @@ > + seed, 32 + 32, &out, 48); > + if (NS_FAILED(rv)) { > + NS_WARNING(nsPrintfCString("OpenVPNSocket::GenerateKeyObjects [this=%p]: " > + "PerformTlsPRF failed.", this).get()); > + return rv; leaks seed and out @@ +1323,5 @@ > + return rv; > + } > + > + if (mAuth != NO_AUTH) { > + nsCString keyMaterial; nsautocstring @@ +1334,5 @@ > + getter_AddRefs(aSession->mHMACDecryptKey)); > + } > + > + if (mCipher != NO_CIPHER) { > + nsCString keyMaterial; nsautocstring @@ +1343,5 @@ > + keyMaterial.Assign(reinterpret_cast<char *>(out2 + 128), mCipherKeySize); > + keyFactory->KeyFromString(mCipher, keyMaterial, > + getter_AddRefs(aSession->mCipherDecryptKey)); > + } > + free(seed); can't these just be declared on the stack? @@ +1378,5 @@ > + this).get()); > + return rv; > + } > + > + if (mProtocol == "udp") { mProtocol.Equals(NS_LITERAL_CSTRING("udp")) @@ +1534,5 @@ > +// i.e., P_CONTROL_HARD_RESET_CLIENT_V2, P_CONTROL_SOFT_RESET_V1 and P_ACK_V1. > +//------------------------------------------------------------------------------ > + > +void > +OpenVPNSocket::SendControlPctNotThroughTls(OpenVPNMsgType aType, can we shorten packet to pkt instead of pct? I read the latter as "percent" @@ +1960,5 @@ > + > +NS_IMETHODIMP > +OpenVPNSocket::Send(char * aBuf, uint32_t aCount, uint32_t *_retval) > +{ > + MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread); the usual thing about asserts and NS_IMETHODIMP idl interfaces ::: netwerk/openvpn/OpenVPNSocket.h @@ +429,5 @@ > + // mTlsChannelFD. > + > + uint64_t mTlsSessionId; > + uint64_t mTlsRemoteSessionId; > + nsTArray<struct TlsSession*> mTlsSubSession; // In case of soft reset, nstarray<nsautoptr<tlssession> > would be a better option here and similarly for msentpackets ::: netwerk/openvpn/TunInterface.h @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef TUN_H__ OPENVPN_TUN_INTERFACE_H (tun_h is begging for a conflict_ @@ +4,5 @@ > + > +#ifndef TUN_H__ > +#define TUN_H__ > + > +#include <net/if.h> it looks to me like the build info sets this up to compile cross platform.. will this compile everywhere? @@ +8,5 @@ > +#include <net/if.h> > +#include "nsITunInterface.h" > +#include "nsASocketHandler.h" > +#include "nsString.h" > +#include "private/pprio.h" private/pprio is terrible - but we do it in other c files.. but does it need to be in an h? @@ +63,5 @@ > + nsCString mRemotePeerVPNAddr; > + uint32_t mTunMtu; > + bool mAttached; > + bool mRunning; > + nsITunInterfaceListener *mCallBack; why isn't this a smartptr? ::: netwerk/openvpn/nsIOpenVPN.idl @@ +39,5 @@ > + /* This function set OpenVPN client into DELETED state. Afterwards it is safe > + * to delete client from the provided list and this client cannot be started > + * anymore. > + */ > + void deleteClient(); destroy? @@ +41,5 @@ > + * anymore. > + */ > + void deleteClient(); > + > + ACString getTunName(); should that be a readonly attribute? @@ +45,5 @@ > + ACString getTunName(); > + > + [noscript] NetAddr getTunAddr(); > + > + bool isRunning(); a ro attribute? @@ +47,5 @@ > + [noscript] NetAddr getTunAddr(); > + > + bool isRunning(); > + > + ACString getRemotePeerAddr(); attribute again.. and localaddr too? @@ +71,5 @@ > + in ACString aRemotePeerVPNAddr); > + > + /** Tls will be used. > + */ > + void setTls(in bool aTls); I think we want to require tls. @@ +75,5 @@ > + void setTls(in bool aTls); > + > + /** Also the Tls packets will be authenticated. > + * Not implemented. > + */ we need non-optional auth. @@ +79,5 @@ > + */ > + void setTlsAuth(in bool aTlsAuth); > + > + /** A security option which is used by default. Each data packet contains a > + * packet ID. if this is something we should do, then why is there a knob for it? (server compatibiltiy?) @@ +83,5 @@ > + * packet ID. > + */ > + void setNoReplay(in bool aNoReplay); > + > + /** Currently not implemented. so this has to be implemented, right? @@ +90,5 @@ > + void setPasswordAuthActive(in bool aActivate); > + > + /** Set Client certificate. > + */ > + void setCertificate(in nsIX509Cert aCert); attribute? @@ +95,5 @@ > + > + attribute nsIInterfaceRequestor securityCallbacks; > + > + /** > + * Set keys to be used in shared secret mode. perhaps we shouldn't be doing shared secret mode.. if it means we can't do auth with tls. Do you have any sense on the popularity of how this is deployed? I'd much rather depend on our tls stack. @@ +102,5 @@ > + in nsIKeyObject aNoTlsCipherDecryptKey, > + in nsIKeyObject aNoTlsHMACEncryptKey, > + in nsIKeyObject aNoTlsHMACDecryptKey); > + > + /** Set authentication algorithm to be used. I would think we wouldn't want to allow anything less than sha1.. and maybe not even that. maybe ask :mt @@ +113,5 @@ > + * const short SHA512 = 6; > + * The value -1 is default which means that packets are not authenticated. > + * nsIOpenVPNSocket::NO_AUTH = -1; > + */ > + void setAuth(in short aAuth); attribute @@ +116,5 @@ > + */ > + void setAuth(in short aAuth); > + > + /** Set cipher. The possible values are defined in nsIOpenVPNSocket > + * const short NO_CIPHER = -1; again, let's get :mt involved.. but definitely do not offer no_cipher as an option. The idea here is to provide some security after all. @@ +122,5 @@ > + * const short AES_192_CBC = 2; > + * const short AES_256_CBC = 3; > + * const short BLOWFISH_CBC = 4; > + */ > + void setCipher(in int32_t aCipher); attribute @@ +124,5 @@ > + * const short BLOWFISH_CBC = 4; > + */ > + void setCipher(in int32_t aCipher); > + > + void setVPNOnlyForNetwork(in ACString aNetAddr, in ACString aNetMask); so that's a destination based route? how does that interact with PAC? @@ +130,5 @@ > + /** Tun interface mtu. The Default value is 1500 bytes. > + */ > + attribute uint32_t tunMtu; > + > + attribute uint8_t routingTableId; what do I do with that? @@ +148,5 @@ > + * OFFLINE_RUNNING means openVPN client is in offline state and will be in > + * RUNNING state as soon as offline state of FF changes. > + */ > + > + void getStatistics (out unsigned long long aDataReadFromTun, should these things just be ro attributes? @@ +155,5 @@ > + out unsigned long long aDataReceiveFromRemotePeer, > + out unsigned long aTimeActive, > + out unsigned long aTimeSinceLastStart); > + > + void NotifyObservers(in unsigned long aState); what does this do? ::: netwerk/openvpn/nsIOpenVPNProvider.idl @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ you say tab-width 4 but you use 2 :).. 2 is better! also adding a vim line keeps nick happy. can you check all the new files for this? ::: netwerk/openvpn/nsIOpenVPNSocket.idl @@ +51,5 @@ > + in ACString aTunVPNAddr, > + in ACString aRemotePeerVPNAddr); > + > + /** Returns remote peer address. > + */ attributes @@ +56,5 @@ > + void getRemotePeerAddr(out ACString aRemotePeerAddr); > + > + /** Returns ip addresses set on the tun interface. > + */ > + void getTunAddrs(out ACString aTunVPNAddr, out ACString aRemotePeerVPNAddr); attributes @@ +60,5 @@ > + void getTunAddrs(out ACString aTunVPNAddr, out ACString aRemotePeerVPNAddr); > + > + /** Tls will be used. > + */ > + void setTls(in bool aTls); as I said earlier.. I'd prefer we insist on tls. do you have a feel for how people deploy these things? @@ +65,5 @@ > + > + /** Also the Tls packets will be authenticated. > + * Not implemented. > + */ > + void setTlsAuth(in bool aTlsAuth); why wouldn't we require this? @@ +73,5 @@ > + */ > + void setNoReplay(in bool aNoReplay); > + > + /** > + * Currently not implemented. gotta do it, right? @@ +89,5 @@ > + > + /** > + * Set Client certificate. > + */ > + void setCertificate(in nsIX509Cert aCert); attribute @@ +106,5 @@ > + * The value -1 is default which means that packets are not authenticated. > + */ > + const short NO_AUTH = -1; > + const short MD5 = 2; > + const short SHA1 = 3; same comments as nsiopenvpn.. need at least sha1 @@ +112,5 @@ > + const short SHA384 = 5; > + const short SHA512 = 6; > + > + void setAuth(in short aAuth); > + attribute @@ +119,5 @@ > + const short AES_192_CBC = 2; > + const short AES_256_CBC = 3; > + const short BLOWFISH_CBC = 4; > + > + /** Sets cipher to be used. lets figure out the strongest we can expect an openvpn server to have and require that @@ +121,5 @@ > + const short BLOWFISH_CBC = 4; > + > + /** Sets cipher to be used. > + */ > + void setCipher(in int32_t aCipher); attribute @@ +127,5 @@ > + /** Get mtu used on the link. > + * MTU for the tun interface can be set (default is 1500 bytes) and mtu for > + * the link is calculated depending on other options used. > + */ > + uint32_t GetLinkMtu(); attribute @@ +133,5 @@ > + /** Tun interface mtu. The Default value is 1500 bytes. > + */ > + attribute uint32_t tunMtu; > + > + unsigned long send(in charPtr aBuf, in unsigned long aCount); document returns @@ +138,5 @@ > + unsigned long recv(in charPtr aBuf, in unsigned long aCount); > + > + /** Set a listener to be notified on openVPN socket state changes. > + */ > + void setStateCallBack(in nsIOpenVPNSocketStateListener aCallBack); attribute @@ +146,5 @@ > + void notifyListener(in unsigned long aState); > + > + /** Set async wait for read and write. > + */ > + void asyncWaitToRead(in nsIOpenVPNSocketReadListener aCallBack); methods of listener? ::: netwerk/openvpn/nsITunInterface.idl @@ +37,5 @@ > + void SetParameters(in ACString aTunVPNAddr, in ACString aRemotePeerVPNAddr); > + > + /** Returns the name of the created Tun interface. > + */ > + ACString getTunName(); attribute @@ +39,5 @@ > + /** Returns the name of the created Tun interface. > + */ > + ACString getTunName(); > + > + NetAddr getTunAddr(); attribute @@ +42,5 @@ > + > + NetAddr getTunAddr(); > + > + attribute uint32_t tunMtu; > + const unsigned long TUN_DEFAULT_MTU = 1500; its not clear to me why the default value needs to appear in the idl @@ +44,5 @@ > + > + attribute uint32_t tunMtu; > + const unsigned long TUN_DEFAULT_MTU = 1500; > + > + bool isRunning(); attribute @@ +46,5 @@ > + const unsigned long TUN_DEFAULT_MTU = 1500; > + > + bool isRunning(); > + > + /** Send data send and received should document their return values and error states @@ +56,5 @@ > + unsigned long recv(in charPtr aBuf, in unsigned long aCount); > + > + /** Set the listener. > + */ > + void setCallBack(in nsITunInterfaceListener aListener); can be an attribute. I like that the other interface is included in this idl. @@ +62,5 @@ > + /** Activates waiting for data to be read. > + * When there is data to be read, onTunReadyToRead will be called and > + * waiting deactivated. > + */ > + void asyncWaitToRead(); should these methods be on the listener? @@ +73,5 @@ > + > + /** Returns the amount of bytes send and receive on tun interface. > + */ > + void transferredBytes(out unsigned long long aReadCount, > + out unsigned long long aWriteCount); stats can probably be attributes @@ +75,5 @@ > + */ > + void transferredBytes(out unsigned long long aReadCount, > + out unsigned long long aWriteCount); > + > + attribute uint8_t routingTableId; what do I do with this? ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1846,5 @@ > nsresult rv; > > + nsRefPtr<LoadContextInfo> info = GetLoadContextInfo(this); > + uint32_t appId; > + info->GetAppId(&appId); you don't really need the ref count stuff here.. GetLoadContextInfo(this)->GetAppId(&appId) ::: netwerk/socket/nsISSLSocketControl.idl @@ +103,5 @@ > * the user or searching the set of rememebered user cert decisions. > */ > attribute nsIX509Cert clientCert; > + > + void enableSessionTickets(in bool aEnable); comments in idls.. and bump the uuid ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +226,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsNSSSocketInfo::EnableSessionTickets(bool aEnable) you'll need dkeeler to loko at the psm changes.. sgtm tho

Thanks for the review > are all those idls things that need to be exposed? typically it would be in > an idl if we wanted chrome to be able to use it or something significantly > out of module.. I had concerns about some things being exposed in them that > I wouldn't blink at if they were just h files. (routing table ids, for > example). I made them idl because I had another idea how to divide and use this code and I left them as idl for that, but now in this form only 2 are needed. I will change that. > > how do we test this.. an example with a pac would be excellent (even if at > first its manual) > I do have a couple of patches only with test in it, I keep them separately. I will attach them next time. > ::: netwerk/base/public/nsIProtocolProxyService2.idl > @@ +35,5 @@ > > */ > > nsICancelable asyncResolve2(in nsIURI aURI, in unsigned long aFlags, > > in nsIProtocolProxyCallback aCallback); > > + > > + nsICancelable asyncResolveWithAppId(in nsIURI aURI, in unsigned long aAppId, > > some documentation is needed.. also bump the uuid > sorry about missing documentation... > ::: netwerk/base/src/ProxyAutoConfig.cpp > @@ +465,5 @@ > > +{ > > + JS::CallArgs args = JS::CallArgsFromVp(argc, vp); > > + > > + if (NS_IsMainThread()) { > > + NS_WARNING("PACMyAppId on Main Thread. How did that happen?"); > > MOZ_ASSERT(false) should we change this in other functions as well (in a separate bug), all these functions, myipaddress, etc., only do NS_WARNING > ::: netwerk/base/src/nsSocketTransport2.cpp > @@ +806,3 @@ > > } > > > > const char *proxyType = nullptr; > > it looks like this declaration can be moved up before the above condition > and then the two conditionals can be combined.. that is true :) > > @@ +1368,5 @@ > > if (status != PR_SUCCESS) { > > return NS_ERROR_FAILURE; > > } > > mBindAddr = nullptr; > > + } else if (fd && mUseVPN) { > > I'm trying to figure out why this is the else() to the if(mBindAddr).. ,that > would seem surprising if you have the vpn.. what if we go down the vpn route > first, and when the vpn code tries to do a bind if there is also an > mBindAddr that also doesn't match we throw an err? > I do not know the intended use of mBindAddr, but for sure there should not be both configured. Throwing an error if they do not match is a good solution. > > @@ +1372,5 @@ > > + } else if (fd && mUseVPN) { > > + nsresult rv; > > + nsCOMPtr<nsIOpenVPNProvider> vpnPrv = > > + do_GetService(NS_OPENVPNPROVIDER_CONTRACTID, &rv); > > + if (NS_SUCCEEDED(rv)) { > > what if it fails? We need to throw an error I think.. that seems true for > all the conditionals in this block. I made it a bit relaxed, but probably you are right. All application that want to use vpn will rely on it. I made it for application that will use vpn if it is there if it is not they do not care, probably there is no such a use case... > > ::: netwerk/openvpn/OpenVPN.cpp > @@ +27,5 @@ > > +namespace net { > > + > > +// The length of queues that hold packets between openVPNSocket > > +// and TunInterface. > > +#define OPENVPN_MAX_QUEUE_LENGTH 2 > > this just seems really small. maybe I misunderstand its role. (and my > comment in the h file about the queue implementation is related.. obviously > if the q is only 2 deep it doesn't matter) > I did not want to make a long queue because the packets are already queued in tun and in socket so we do not need to buffer it again and this really short queue it is only to compensate the fluctuation when tun is ready for read/write and when socket is ready for read/write. Maybe it should be longer, but i am not sure if we are going to notice a difference at all...maybe only during key renewing. it can be also one just 1 packet and we can save memory allocations. > @@ +54,5 @@ > > + WaitOpenVPNToCleanUp(nsIOpenVPN *aOVPN) > > + : mOVPN(aOVPN) > > + {} > > + > > + NS_IMETHOD Run() > > the whole point of this is to do the release on the mainthread? > > you can do that with nsMainThreadPtrHolder<nsIOpenVPN> much easier and > better. > Not on the mainthead but on socketthread. It is waiting in case OpenVPNSocket or TunInterface or SocketTransoprtService dispatch some calls during OpenVPNSocket->close() and TunInterface->close() (they all will be on socketthread). > @@ +94,5 @@ > > +} > > + > > +OpenVPN::~OpenVPN() > > +{ > > + MOZ_ASSERT(mState == nsIOpenVPN::DELETED); > > should it just call delete() itself if not deleted? We can't really assert > on proper use of an idl.. callers will be dumb. This was more for me to be sure everything is closed properly and deleted. If if everything is shut down properly and nobody changes anything in IOService or just do something stupid like "delete" this should not happen. > > @@ +122,5 @@ > > + if (PR_GetCurrentThread() != gSocketThread) { > > + mTarget->Dispatch(NS_NewRunnableMethod(this, > > + &nsIOpenVPN::Start), > > + NS_DISPATCH_NORMAL); > > + return NS_OK; > > so I'm worried about start() and a race condition with offline (or deleted). > > main thread calls start.. its online and the new runnable-start gets > dispatched to the socket thread, and them main thread gets ok. > > now we go offline > > now runnable-start executes, but it returns NS_ERROR_NOT_AVAILABLE because > we're offline. but the caller of Start() can't know this as its no longer on > the stack. > Actualy the call needs to listen on openvpnstate. The listeners will be notified about the real and correct state of openvpn client. In your example start will not change state at all. on go offline the listener will be notified that that openvpn client is offline so it knows the openvpn client have not started at all. I will move ChangeState(nsIOpenVPN::INITIALIZING); before dispatch so that a listener is notified that something called start. And in you example offline state then will be offline_running which is correct and not offline_stop like now. > > @@ +188,5 @@ > > + > > + while (mFromTunToPeer.Length()) { > > + struct PacketBuffer* pct = mFromTunToPeer.LastElement(); > > + mFromTunToPeer.RemoveElementAt(mFromTunToPeer.Length() - 1); > > + free(pct); > > The queues would be better defined (if we keep them as arrays) as > nsTArray<nsAutoPtr<PacketBuffer> > .. then normal c++ dtors will prevent > leaks and we can get rid of the manual frees. it would just be > mFromTunToPeer.Clear() if we needed to clear them outside of ~openvpn() > Thanks, I will change this, i did not know that nsAutoPtr exists, I have missed it somehow, there is a lot of new stuff for me. > > @@ +365,5 @@ > > + > > + return mTun->SetParameters(aTunVPNAddr, aRemotePeerVPNAddr); > > +} > > + > > +// Not implemented. > > why not? (why have it if not implemented..) > It is a function that i wanted to discuss with you and with whom ever is going to use this if we need it or not. All "not implemented" are place holders to see if we need it or not. For know the only thing implemented is to configure vpn wit PAC like: "vpn vpnserver.com" it can depend on appid (myAppId) and all traffic for a app will be routed through vpn This function would be exposed in ui and user can configure that packets are routed over vpn only if the destination is in certain network. The use case that we know till now do not need this. I will delete it and if somebody needs it we can implement it. > > @@ +740,5 @@ > > + mTun->SetCallBack(this); > > + nsresult rv = mTun->Init(); > > + if (NS_FAILED(rv)) { > > + ChangeState(nsIOpenVPN::ERROR); > > + StopInternal(true); > > shoul this return? Yes, it should. > > @@ +42,5 @@ > > + void ChangeStateWithLock(uint32_t aNewState); > > + bool IsOffline(); > > + friend class VPNStateChanged; > > + struct PacketBuffer { > > + char *mBuf; > > you can use nsAutoArrayPtr<char> here to have the dtor do the cleanup c++ > style ok, thanks. > > @@ +85,5 @@ > > + bool mPacketWaitingFromTun; > > + uint32_t mMtuToTun; > > + nsCOMPtr<nsIEventTarget> mTarget; > > + > > + PRIntervalTime mLastStart; > > there are some real range limitations on printervatime and its hard to use > correctly. use mozilla::timestamp instead. ok, thanks. > > ::: netwerk/openvpn/OpenVPNProvider.cpp > @@ +25,5 @@ > > +// OpenVPN > > +//------------------------------------------------------------------------------ > > + > > +OpenVPNProvider::OpenVPNProvider() > > + : mLock("OpenVPNProvider access lock") > > so we've got a lock which implies cross thread access. can you explain what > methods are expected to be called from what threads? > OpenVPNProvider will be access on main thread from js, in IOService (which is on the main thread) and in SocketTransport to find vpn client and binding address on SocketThread. > @@ +62,5 @@ > > + > > + nsRefPtr<OpenVPN> vpn = new OpenVPN(); > > + > > + vpn->SetRoutingTableId(252 - inx); // 253 is default table. Table index > > + // between 252 and 252. > > you mean 252 - MAXROUTINGTABLE.. isn't this really fragile and depends on > local config state? I had in mind an easy implementation: to reserve a couple of routing tables for vpn, and write them in advance into rt_tables.. I will change this in next version and add parsing and writing into rt_tables. I will probably extend SetRoute function to parse error if there is any. For now I have assumed that nobody is messing up with routing tables :) > > @@ +123,5 @@ > > +OpenVPNProvider::GetTunNameForLocalVPNAddr(const nsACString &aLocalAddr, > > + nsACString &aTunName) > > +{ > > + MutexAutoLock lock(mLock); > > +/* > > why is this not working? It is also one of the stuff that is not used currently (there is an explanation in OpenVPNProvider.idl). The idea of this one it to configure PAC with tun interface address or network. VPN 10.0.0.0/24 I will remove it and if use case needs it I will add it... > ::: netwerk/openvpn/OpenVPNProvider.h > @@ +21,5 @@ > > + > > +// The maximal number of routing table ids. This will be the maximal number of > > +// clients that can be configured at a time. TODO discuss probably 10 is too > > +// much? > > +#define MAXROUTINGTABLE 10 > > why do we need a max? > just for easier implementation, i will change this, see above. > @@ +433,5 @@ > > + : mOpenVPNSocket(aOpenVPNSocket) > > + , mState(aState) > > + {} > > + > > + NS_IMETHOD Run() > > assert the thread you expect run to be on.. useful for documentation if > nothing else ok. > > @@ +904,5 @@ > > + > > +NS_IMETHODIMP > > +OpenVPNSocket::GetTunMtu(uint32_t *aTunMtu) > > +{ > > + MOZ_ASSERT(aTunMtu); > > if you keep this as an IDL you want to make that NS_ENSURE_ARG_POINTER.. we > can't assert that some other module follows the calling pattern, we can only > do that internal to necko code. (the former will return an err if its > null).. this applies to a bunch of methods here. > Thanks for the explanation. > otoh if the xpcom idl goes away you can and should assert that the rest of > necko is being sane. > > @@ +1019,5 @@ > > + if (NS_FAILED(rv)) { > > + NS_WARNING(nsPrintfCString("OpenVPNSocket::CreateNewSession [this=%p]: " > > + "Failed creating the random number generator, " > > + "use rand.", this).get()); > > + *sessionId = ((uint64_t)rand() << 32) + (uint64_t)rand(); > > I'm going to say we should refuse to create the session if the trusted rng > isn't available. ok, fine by me :). > > @@ +1216,5 @@ > > + return rv; > > +} > > + > > +void > > +OpenVPNSocket::SetRandomKeys(struct OpenVPNSocket::TlsSession *aSession) > > any failures here ought to be really a vpn failure, right? The same thing as above. > > @@ +1534,5 @@ > > +// i.e., P_CONTROL_HARD_RESET_CLIENT_V2, P_CONTROL_SOFT_RESET_V1 and P_ACK_V1. > > +//------------------------------------------------------------------------------ > > + > > +void > > +OpenVPNSocket::SendControlPctNotThroughTls(OpenVPNMsgType aType, > > can we shorten packet to pkt instead of pct? I read the latter as "percent" > Ok, no problem. > ::: netwerk/openvpn/OpenVPNSocket.h > @@ +429,5 @@ > > + // mTlsChannelFD. > > + > > + uint64_t mTlsSessionId; > > + uint64_t mTlsRemoteSessionId; > > + nsTArray<struct TlsSession*> mTlsSubSession; // In case of soft reset, > > nstarray<nsautoptr<tlssession> > would be a better option here and similarly > for msentpackets > I will change this > @@ +4,5 @@ > > + > > +#ifndef TUN_H__ > > +#define TUN_H__ > > + > > +#include <net/if.h> > > it looks to me like the build info sets this up to compile cross platform.. > will this compile everywhere? > This will be FF OS only so I will put #ifdef MOZ_WIDGET_GONK as soon as we finish first review. It is just easier to test it on linux :) Now it is working on emulator and on my local machine (with changing "/dev/net/tun" into "/dev/tun") > ::: netwerk/openvpn/nsIOpenVPN.idl > @@ +39,5 @@ > > + /* This function set OpenVPN client into DELETED state. Afterwards it is safe > > + * to delete client from the provided list and this client cannot be started > > + * anymore. > > + */ > > + void deleteClient(); > > destroy? > It is a better name. > @@ +41,5 @@ > > + * anymore. > > + */ > > + void deleteClient(); > > + > > + ACString getTunName(); > > should that be a readonly attribute? > you are right for all of this functions... > > @@ +71,5 @@ > > + in ACString aRemotePeerVPNAddr); > > + > > + /** Tls will be used. > > + */ > > + void setTls(in bool aTls); > > I think we want to require tls. > I did implement almost all configuration option that are offered in the original openVPN client Most servers use tls. The other option is shared-secret which is easier to configure (tutorials usually state that it is easier to configure, but it is not really true). I notice from blogs and posts that if people want a quick setup of a vpn they use share-secret but only (I think only) for private use. If we decide to keep only tls, i will keep a patch for shared-secret I do not want to go through this all over again and try to figure out how it works > @@ +75,5 @@ > > + void setTls(in bool aTls); > > + > > + /** Also the Tls packets will be authenticated. > > + * Not implemented. > > + */ > > we need non-optional auth. > This is a stupid one and i have not seen any server use it. It uses pre-shared secret to authenticate a complete tls packet. > @@ +79,5 @@ > > + */ > > + void setTlsAuth(in bool aTlsAuth); > > + > > + /** A security option which is used by default. Each data packet contains a > > + * packet ID. > > if this is something we should do, then why is there a knob for it? (server > compatibiltiy?) > It is an configuration option for the original openvpn server. > @@ +83,5 @@ > > + * packet ID. > > + */ > > + void setNoReplay(in bool aNoReplay); > > + > > + /** Currently not implemented. > > so this has to be implemented, right? > It is implemented I just need to test it. There is a lot of configurations and failure cases to test, and this one takes some time to configure the server side so i just have postponed it and there is enough implemented to review in the first iteration. :) > > @@ +95,5 @@ > > + > > + attribute nsIInterfaceRequestor securityCallbacks; > > + > > + /** > > + * Set keys to be used in shared secret mode. > > perhaps we shouldn't be doing shared secret mode.. if it means we can't do > auth with tls. Do you have any sense on the popularity of how this is > deployed? I'd much rather depend on our tls stack. > Share secret do not do any auth with tls. > @@ +102,5 @@ > > + in nsIKeyObject aNoTlsCipherDecryptKey, > > + in nsIKeyObject aNoTlsHMACEncryptKey, > > + in nsIKeyObject aNoTlsHMACDecryptKey); > > + > > + /** Set authentication algorithm to be used. > > I would think we wouldn't want to allow anything less than sha1.. and maybe > not even that. maybe ask :mt > ok. I will. What kind of authentication and ciphers we will offer is the next thing we should decide. > > ::: netwerk/openvpn/nsIOpenVPNProvider.idl > @@ +1,1 @@ > > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ > > you say tab-width 4 but you use 2 :).. 2 is better! also adding a vim line > keeps nick happy. can you check all the new files for this? > If it keeps Nick happy... :) > > + void setTlsAuth(in bool aTlsAuth); > > why wouldn't we require this? It is not very good name, as i wrote above it uses a pre-shared secret to authenticate complete TLS packets again. > > @@ +146,5 @@ > > + void notifyListener(in unsigned long aState); > > + > > + /** Set async wait for read and write. > > + */ > > + void asyncWaitToRead(in nsIOpenVPNSocketReadListener aCallBack); > > methods of listener? This is the listener saying it is ready to accept packets. > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +1846,5 @@ > > nsresult rv; > > > > + nsRefPtr<LoadContextInfo> info = GetLoadContextInfo(this); > > + uint32_t appId; > > + info->GetAppId(&appId); > > you don't really need the ref count stuff here.. > GetLoadContextInfo(this)->GetAppId(&appId) > ok I started adding ipv6 support so i will finish it and I will fix the stuff mention in these comments. There will be a new version on Monday

dd - you might look at mozilla::vector too for some of the buffer management. Its new to me too.

Hi Martin, I am implementing OpenVPN client for ff OS. Patrick told me to ask you about what kind of security, authentication and ciphers we should support. 1) There are 2 main security option in openvpn server/client, both are implemented: static key (shared-secret) - the keys for encryption and authentication are know prior to connecting to a openvpn server. Tls is not use here at all. I have not noticed any publicly available openvpn servers using this. According to posts and blogs most people that are using static key mode, use it for private servers, because it should be easier to configure. the other option is using tls to exchange keys and authenticate. Should we offer static key mode at all? 2) Which cipher should we offer? The latest openvpn (2.3.5 since 28.October 2014) offers following: $ openvpn --show-ciphers The following ciphers and cipher modes are available for use with OpenVPN. Each cipher shown below may be used as a parameter to the --cipher option. The default key size is shown as well as whether or not it can be changed with the --keysize directive. Using a CBC mode is recommended. In static key mode only CBC mode is allowed. DES-CFB 64 bit default key (fixed) (TLS client/server mode) DES-CBC 64 bit default key (fixed) RC2-CBC 128 bit default key (variable) RC2-CFB 128 bit default key (variable) (TLS client/server mode) RC2-OFB 128 bit default key (variable) (TLS client/server mode) DES-EDE-CBC 128 bit default key (fixed) DES-EDE3-CBC 192 bit default key (fixed) DES-OFB 64 bit default key (fixed) (TLS client/server mode) DES-EDE-CFB 128 bit default key (fixed) (TLS client/server mode) DES-EDE3-CFB 192 bit default key (fixed) (TLS client/server mode) DES-EDE-OFB 128 bit default key (fixed) (TLS client/server mode) DES-EDE3-OFB 192 bit default key (fixed) (TLS client/server mode) DESX-CBC 192 bit default key (fixed) BF-CBC 128 bit default key (variable) BF-CFB 128 bit default key (variable) (TLS client/server mode) BF-OFB 128 bit default key (variable) (TLS client/server mode) RC2-40-CBC 40 bit default key (variable) CAST5-CBC 128 bit default key (variable) CAST5-CFB 128 bit default key (variable) (TLS client/server mode) CAST5-OFB 128 bit default key (variable) (TLS client/server mode) RC2-64-CBC 64 bit default key (variable) AES-128-CBC 128 bit default key (fixed) AES-128-OFB 128 bit default key (fixed) (TLS client/server mode) AES-128-CFB 128 bit default key (fixed) (TLS client/server mode) AES-192-CBC 192 bit default key (fixed) AES-192-OFB 192 bit default key (fixed) (TLS client/server mode) AES-192-CFB 192 bit default key (fixed) (TLS client/server mode) AES-256-CBC 256 bit default key (fixed) AES-256-OFB 256 bit default key (fixed) (TLS client/server mode) AES-256-CFB 256 bit default key (fixed) (TLS client/server mode) AES-128-CFB1 128 bit default key (fixed) (TLS client/server mode) AES-192-CFB1 192 bit default key (fixed) (TLS client/server mode) AES-256-CFB1 256 bit default key (fixed) (TLS client/server mode) AES-128-CFB8 128 bit default key (fixed) (TLS client/server mode) AES-192-CFB8 192 bit default key (fixed) (TLS client/server mode) AES-256-CFB8 256 bit default key (fixed) (TLS client/server mode) DES-CFB1 64 bit default key (fixed) (TLS client/server mode) DES-CFB8 64 bit default key (fixed) (TLS client/server mode) DES-EDE3-CFB1 192 bit default key (fixed) (TLS client/server mode) DES-EDE3-CFB8 192 bit default key (fixed) (TLS client/server mode) CAMELLIA-128-CBC 128 bit default key (fixed) CAMELLIA-192-CBC 192 bit default key (fixed) CAMELLIA-256-CBC 256 bit default key (fixed) CAMELLIA-128-CFB 128 bit default key (fixed) (TLS client/server mode) CAMELLIA-192-CFB 192 bit default key (fixed) (TLS client/server mode) CAMELLIA-256-CFB 256 bit default key (fixed) (TLS client/server mode) CAMELLIA-128-CFB1 128 bit default key (fixed) (TLS client/server mode) CAMELLIA-192-CFB1 192 bit default key (fixed) (TLS client/server mode) CAMELLIA-256-CFB1 256 bit default key (fixed) (TLS client/server mode) CAMELLIA-128-CFB8 128 bit default key (fixed) (TLS client/server mode) CAMELLIA-192-CFB8 192 bit default key (fixed) (TLS client/server mode) CAMELLIA-256-CFB8 256 bit default key (fixed) (TLS client/server mode) CAMELLIA-128-OFB 128 bit default key (fixed) (TLS client/server mode) CAMELLIA-192-OFB 192 bit default key (fixed) (TLS client/server mode) CAMELLIA-256-OFB 256 bit default key (fixed) (TLS client/server mode) SEED-CBC 128 bit default key (fixed) SEED-OFB 128 bit default key (fixed) (TLS client/server mode) SEED-CFB 128 bit default key (fixed) (TLS client/server mode) In previous version 2.3.4 all OFB and CFB ciphers were broken. 3) And also authentication: $ openvpn --show-digests The following message digests are available for use with OpenVPN. A message digest is used in conjunction with the HMAC function, to authenticate received packets. You can specify a message digest as parameter to the --auth option. MD5 128 bit digest size RSA-MD5 128 bit digest size SHA 160 bit digest size RSA-SHA 160 bit digest size SHA1 160 bit digest size RSA-SHA1 160 bit digest size DSA-SHA 160 bit digest size DSA-SHA1-old 160 bit digest size DSA-SHA1 160 bit digest size RSA-SHA1-2 160 bit digest size DSA 160 bit digest size RIPEMD160 160 bit digest size RSA-RIPEMD160 160 bit digest size MD4 128 bit digest size RSA-MD4 128 bit digest size ecdsa-with-SHA1 160 bit digest size RSA-SHA256 256 bit digest size RSA-SHA384 384 bit digest size RSA-SHA512 512 bit digest size RSA-SHA224 224 bit digest size SHA256 256 bit digest size SHA384 384 bit digest size SHA512 512 bit digest size SHA224 224 bit digest size whirlpool 512 bit digest size 4) openvpn server gives a possibility to turn off encryption and authentication, tls handshake (in not shared key mode) is still there but application packet are not authenticated and encrypted. Should we insist on an authentication and encryption?

One more special thing about openvpn servers: The way to prevent plaintext attacks in tls < 1.1 is to split application records into 2 records and the first one only contains the first byte of the plain text. This is in by default, but this causes openvpn server to close. It is possible to turn this off with NSS_SSL_CBC_RANDOM_IV=0. Is there any other way to turn this off, so that it can be turn off per connection?

Looks like you can do this: SSL_OptionSet(fd, SSL_CBC_RANDOM_IV, true); (From https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl.h#136 ) That sounds like a bug in openvpn that should be fixed, though.

(In reply to Dragana Damjanovic from comment #13) > Hi Martin, > I am implementing OpenVPN client for ff OS. Patrick told me to ask you about > what kind of security, authentication and ciphers we should support. > > 1) There are 2 main security option in openvpn server/client, both are > implemented: > static key (shared-secret) - the keys for encryption and authentication > are know prior to connecting to a openvpn server. Tls is not use here at > all. I have not noticed any publicly available openvpn servers using this. > According to posts and blogs most people that are using static key mode, use > it for private servers, because it should be easier to configure. > > the other option is using tls to exchange keys and authenticate. > > Should we offer static key mode at all? TLS only please. We should only offer the static key mode if there is a strong case for it (large deployments or a large number of deployments that can't support TLS where the only alternative is much worse). For a new feature like this, we need to start with the best security available. > 2) Which cipher should we offer? The latest openvpn (2.3.5 since 28.October > 2014) offers following: I think that you want to limit this to AES and maybe CAMELLIA and SEED. The CBC modes are probably sufficient. Definitely not RC2 or DES. I'm assuming that if you use TLS, you get TLS cipher suites instead and that those will be vastly superior to this lot. Assuming of course that you are using a recent version of openssl or nss or something equivalently good. > 3) And also authentication: Ummmm, none of these? I'm assuming that if you use TLS, you get TLS cipher suites instead. What matters here is the security level of the signature and the strength of the hash function. The RS and ECDSA modes are most likely to be OK, but SHA-1 is pretty miserable. I assume that the straight hash-based methods are using HMAC, which is OK-ish, because you are only relying on second preimage resistance, and even HMAC-MD5 isn't completely broken there (I wouldn't rely on that being true though). Anything RSA or ECDSA with SHA256 or stronger is maybe OK. The rest: suspect. > 4) openvpn server gives a possibility to turn off encryption and > authentication, tls handshake (in not shared key mode) is still there but > application packet are not authenticated and encrypted. Should we insist on > an authentication and encryption? Oh yes. Overall, if we pursue this, I think that we might want to consider investing some resources in making it easier to use good (TLS-based) crypto for OpenVPN. Is there a page that explains how trust anchors are established for this feature? I assume that you *don't* want to be using the standard domain PKIX anchors for your VPN.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #15) > Looks like you can do this: SSL_OptionSet(fd, SSL_CBC_RANDOM_IV, true); > (From > https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl. > h#136 ) > That sounds like a bug in openvpn that should be fixed, though. Yes, I'd tweak the fd rather than try to disable BEAST splitting entirely for the whole browser. The splitting thing isn't needed for something like a VPN, since it is highly unlikely to be vulnerable to a BEAST-like attack, but patching openvpn (see above) is still a good idea.

(In reply to Martin Thomson [:mt] from comment #17) > (In reply to David Keeler (:keeler) [use needinfo?] from comment #15) > > Looks like you can do this: SSL_OptionSet(fd, SSL_CBC_RANDOM_IV, true); > > (From > > https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl. > > h#136 ) > > That sounds like a bug in openvpn that should be fixed, though. > > Yes, I'd tweak the fd rather than try to disable BEAST splitting entirely > for the whole browser. The splitting thing isn't needed for something like > a VPN, since it is highly unlikely to be vulnerable to a BEAST-like attack, > but patching openvpn (see above) is still a good idea. Thanks Martin and David, I will see about patching openvpn,I think they do not touch ssl stuff at all they just use a library. Anyway i will check it out.

(In reply to Martin Thomson [:mt] from comment #16) > > > > Should we offer static key mode at all? > > TLS only please. > > We should only offer the static key mode if there is a strong case for it > (large deployments or a large number of deployments that can't support TLS > where the only alternative is much worse). For a new feature like this, we > need to start with the best security available. > ok we will support only tls mode for now. > > 2) Which cipher should we offer? The latest openvpn (2.3.5 since 28.October > > 2014) offers following: > > I think that you want to limit this to AES and maybe CAMELLIA and SEED. The > CBC modes are probably sufficient. Definitely not RC2 or DES. > > I'm assuming that if you use TLS, you get TLS cipher suites instead and that > those will be vastly superior to this lot. Assuming of course that you are > using a recent version of openssl or nss or something equivalently good. > I should have explained it a bit more. In tls mode they have 2 channel: control channel and data channel. Control channel uses tls. After tls handshake it is only use for keys exchange. This is not a problem because i just use our standart ssl layer. Data channel is used for application data. It is does not use tls and it is not dtls but packets are encrypted and authenticated using one of ciphers and authentication algorithms listed in comment 13 so we should support some of them. > > 3) And also authentication: > > Ummmm, none of these? I'm assuming that if you use TLS, you get TLS cipher > suites instead. > > What matters here is the security level of the signature and the strength of > the hash function. The RS and ECDSA modes are most likely to be OK, but > SHA-1 is pretty miserable. I assume that the straight hash-based methods > are using HMAC, which is OK-ish, because you are only relying on second > preimage resistance, and even HMAC-MD5 isn't completely broken there (I > wouldn't rely on that being true though). > > Anything RSA or ECDSA with SHA256 or stronger is maybe OK. The rest: > suspect. > ok. > > Overall, if we pursue this, I think that we might want to consider investing > some resources in making it easier to use good (TLS-based) crypto for > OpenVPN. > I read about a possibility of switching openvpn to use dtls instead their own something like dtls over udp, but I have not seen much postings recently and I do not know if they are working on it at all. > Is there a page that explains how trust anchors are established for this > feature? I assume that you *don't* want to be using the standard domain > PKIX anchors for your VPN. Currently I am using the standard domain, but I am not really familiar with this who should I ask for help?

(In reply to Dragana Damjanovic from comment #19) > I should have explained it a bit more. In tls mode they have 2 channel: > control channel and data channel. > Control channel uses tls. After tls handshake it is only use for keys > exchange. This is not a problem because i just use our standart ssl layer. > Data channel is used for application data. It is does not use tls and it is > not dtls but packets are encrypted and authenticated using one of ciphers > and authentication algorithms listed in comment 13 so we should support some > of them. Is there a spec that you can refer us to? Key derivation and how it is used is important. (DTLS would be a really good idea, if that is possible.) > Currently I am using the standard domain, but I am not really familiar with > this who should I ask for help? :keeler probably knows more of the mechanics than I here regarding installing of different trust anchors. If you want to use certificate fingerprints, then I can point you in the right direction.

Dragana, maybe should you require from some other people a feedback/cancel as this was canceled?

This project is paused for now, because we do not have a user. If we have a user for it, interest, we would finish it.

What do you mean you don't have users? I am in Iran, we need VPN here! Chinese need VPN on their OS or they can't do much neither...

This feature is only for firefox os because it needs root access rights to work. We will land this just currently its priority is not high. The user interface is missing and that is more work than finishing this patch which implements the firefox platform part.

I don't understand everything but I confirm that I am using a Nexus 5 with FFOS and an openVPN option would be great! If Mozilla plan to launch FFOS in China (Iran is not a big market, but I'll work on the persian keyboard and FFOS translation to persian language), VPN is a need there!

OpenVPN is basically useless in China. Leaping over the GFW requires more sophisticated systems.

I was using it three years ago when I was there but things might have changed... Sorry, I didn't know.

Some discussion about the problems with VPN in China and the Iran: https://news.ycombinator.com/item?id=10101469 But I would appreciate VPN support on FxOS, so I can access the company network or use the VPN tunnel provided from our Freifunk inititive when using a public WiFi.

Thank you for these terrible news about restriction access to Internet for my chinese friends, and well for me for the year coming... I'm not developer, so, well, we need you mozilians and relatives!!!! I can't stop saying thank you to all the great you do for Internet.

I am unassigning me, because i will not work on this in the near future.

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258