Hi Nathaniel, Here is an updated password patch. This should alleviate your fears on what you called "hand-rolled crypto", as it uses plain HMAC (which also makes the code smaller).
Changes: * uses HMAC rather than MD5 directly * deals with short writes to the socket * stores the password in a text file so that it cannot be snooped from the process list. This has the added benefit of allowing us to do things like running xpra using the same password file for authentication with Xvnc: xpra --xvfb-bin="Xvnc -PasswordFile=/path/passwordfile" --password-file=/path/passwordfile start :10 This obviously requires my previous patch to be merged (the trivial one allowing "--xvfb-bin" option on the command line). Cheers Antoine Nathaniel Smith wrote: > On Mon, Jun 15, 2009 at 12:02 AM, Antoine Martin<[email protected]> wrote: >> Nathaniel Smith wrote: >>> On Sun, Jun 14, 2009 at 8:42 AM, Antoine Martin<[email protected]> >>> wrote: >>>> This patch implements secure passwords authentication (I believe - can >>>> someone please check!) >>> Well, that's the problem with hand-rolled crypto -- it's mostly >>> impossible to check... >> Hard but not impossible! > [...analysis snipped...] > > Well, I meant what I said :-). I actually have a fair amount of crypto > expertise. The sad thing about crypto expertise is that you learn a > whole bunch of cool things, and then you can't actually do anything > with it, because the more you study the more you realize that the > *only* way to write working crypto designs is to (1) be one of the > best people in the world at this stuff, AND (2) convince all the other > best people in the world to spend a few years analyzing your design. > They're still finding design flaws in *ssh*, for crissakes. > > So maybe this design is fine, but the problem isn't just making > something that works, it's being confident that we can stand by it and > support it. I don't see any problems in your analysis, but that > doesn't convince me that there aren't any. I need a really good reason > to take that risk. (Plus, how do I know that the next person won't > want even more assurances, like encryption or MITM protection or > whatever? That will *definitely* spiral out of our ability to get > right. And that also means that whatever reason is given needs to > explain why we need a simple ad-hoc password scheme AND NO MORE.) > >>> Technically, the patch looks reasonable enough, and things like the >>> short write issue are fixable. >> I'm curious, can you suggest how? >> Without adding a whole lot of code that is... I was trying to keep it small. > > It's just a little loop like > written = 0 > while written < len(data): > select.select([], [sock], []) > written += sock.write(data[written:]) > (though of course it's better to wrap this in a utility function) > >>> I'm *really* nervous, though, about getting into the secure connection >>> business. That's an impossibly difficult problem, and it's one that's >>> already solved -- just use ssh. So I'm reluctant to accept any patches >>> along these lines without, at the least, specific use cases and clear >>> argumentation for why this approach is the most appropriate way to >>> handle those use cases. Can you provide such? >> I think I can. I've got Windows boxes on the LAN, these will not have >> openssh or plink installed. > > plink is trivial to install (literally: "click 'Save link as...', > done"). Hardly noticeable compared to installing the rest of xpra > (which is much less trivial). > >> Another use case would be anonymous sessions (ie: locked down guest) >> where you want to allow access from remote hosts without giving them >> full ssh access. (it may also be do-able by setting their login shell to >> an xpra wrapper, but that's just too limited and nasty) > > This seems like a very specific and somewhat odd use case -- is this > actually something you are planning to do for real? > > (Note also that it's trivial to configure ssh so that when a certain > key is used it will automatically spawn some particular program rather > than give general shell access -- google "ssh restrict commands" or > somesuch.) > >> BTW, I'm even trying to build the xpra server for win32 now, any ideas >> where I could find one that builds/works?? >> (and the machines that will run it will definitely not be running >> openssh server but will still require authentication of some sort) > > Cygwin comes with an openssh service. You may well want to run xpra > under cygwin anyway (that'd be the simplest way to get the named pipe > support that xpra assumes!), so installing cygwin may not be an extra > burden at all. And if you must start running a new network-accessible > service on this machine, you should trust openssh a heck of a lot more > than any code that we throw together ourselves. > > Cheers, > -- Nathaniel
diff -ur upstream/xpra/client.py password_file/xpra/client.py
--- upstream/xpra/client.py 2009-07-28 20:14:42.000000000 +0700
+++ password_file/xpra/client.py 2009-07-28 20:19:35.000000000 +0700
@@ -8,6 +8,8 @@
import cairo
import os
import os.path
+import sys
+import hmac
from wimpiggy.util import (one_arg_signal,
gtk_main_quit_really,
@@ -268,17 +270,16 @@
gobject.type_register(ClientWindow)
class XpraClient(gobject.GObject):
- def __init__(self, sock, compression_level):
+ def __init__(self, sock, compression_level, password_file):
gobject.GObject.__init__(self)
self._window_to_id = {}
self._id_to_window = {}
+ self.password_file = password_file
+ self.compression_level = compression_level
self._protocol = Protocol(sock, self.process_packet)
ClientSource(self._protocol)
- capabilities_request = dict(default_capabilities)
- if compression_level:
- capabilities_request["deflate"] = compression_level
- self.send(["hello", capabilities_request])
+ self.send_hello()
self._keymap = gtk.gdk.keymap_get_default()
self._keymap.connect("keys-changed", self._keys_changed)
@@ -315,6 +316,25 @@
def send_mouse_position(self, packet):
self._protocol.source.queue_mouse_position_packet(packet)
+ def send_hello(self, hash=None):
+ capabilities_request = dict(default_capabilities)
+ if hash:
+ capabilities_request["challenge_response"] = hash
+ if self.compression_level:
+ capabilities_request["deflate"] = self.compression_level
+ self.send(["hello", capabilities_request])
+
+ def _process_challenge(self, packet):
+ if not self.password_file:
+ log.error("password is required by the server")
+ gtk.main_quit()
+ return
+ passwordFile = open(self.password_file, "rU")
+ password = passwordFile.read()
+ (_, salt) = packet
+ hash = hmac.HMAC(password, salt)
+ self.send_hello(hash.hexdigest())
+
def _process_hello(self, packet):
(_, capabilities) = packet
if "deflate" in capabilities:
@@ -367,6 +387,7 @@
_packet_handlers = {
"hello": _process_hello,
+ "challenge": _process_challenge,
"new-window": _process_new_window,
"new-override-redirect": _process_new_override_redirect,
"draw": _process_draw,
diff -ur upstream/xpra/scripts/main.py password_file/xpra/scripts/main.py
--- upstream/xpra/scripts/main.py 2009-07-28 20:17:35.000000000 +0700
+++ password_file/xpra/scripts/main.py 2009-07-28 20:20:38.000000000 +0700
@@ -53,6 +53,9 @@
dest="bind_tcp", default=None,
metavar="[HOST]:PORT",
help="Listen for connections over TCP (insecure)")
+ parser.add_option("--password-file", action="store",
+ dest="password_file", default=None,
+ help="The file containing the password required to connect (useful to secure TCP mode)")
parser.add_option("-z", "--compress", action="store",
dest="compression_level", type="int", default=3,
metavar="LEVEL",
@@ -158,7 +161,7 @@
sock, local = client_sock(parser, opts, pick_display(parser, extra_args))
if opts.compression_level < 0 or opts.compression_level > 9:
parser.error("Compression level must be between 0 and 9 inclusive.")
- app = XpraClient(sock, opts.compression_level)
+ app = XpraClient(sock, opts.compression_level, opts.password_file)
sys.stdout.write("Attached\n")
app.run()
diff -ur upstream/xpra/scripts/server.py password_file/xpra/scripts/server.py
--- upstream/xpra/scripts/server.py 2009-06-18 03:20:38.000000000 +0700
+++ password_file/xpra/scripts/server.py 2009-07-28 20:26:49.000000000 +0700
@@ -286,7 +286,7 @@
# This import is delayed because the module depends on gtk:
import xpra.server
- app = xpra.server.XpraServer(upgrading, sockets)
+ app = xpra.server.XpraServer(upgrading, sockets, opts.password_file)
def cleanup_socket(self):
print "removing socket"
try:
diff -ur upstream/xpra/server.py password_file/xpra/server.py
--- upstream/xpra/server.py 2009-06-18 03:20:40.000000000 +0700
+++ password_file/xpra/server.py 2009-07-28 20:23:47.000000000 +0700
@@ -207,7 +207,7 @@
"wimpiggy-child-map-event": one_arg_signal,
}
- def __init__(self, clobber, sockets):
+ def __init__(self, clobber, sockets, password_file):
gobject.GObject.__init__(self)
# Do this before creating the Wm object, to avoid clobbering its
@@ -289,6 +289,10 @@
self._has_focus = 0
self._upgrading = False
+ ### Password authentication
+ self.password_file = password_file
+ self.salt = None
+
### All right, we're ready to accept customers:
self._protocol = None
self._potential_protocols = []
@@ -483,7 +487,7 @@
def _calculate_capabilities(self, client_capabilities):
capabilities = {}
- for cap in ("deflate", "__prerelease_version"):
+ for cap in ("deflate", "__prerelease_version", "challenge_response"):
if cap in client_capabilities:
capabilities[cap] = client_capabilities[cap]
return capabilities
@@ -497,6 +501,34 @@
+ "of exactly the same version (v%s)", xpra.__version__)
proto.close()
return
+ # Password required?
+ if self.password_file:
+ log.debug("password auth required")
+ client_hash = capabilities.get("challenge_response")
+ if not client_hash or not self.salt:
+ self.salt = "%s" % uuid.uuid4()
+ capabilities["challenge"] = self.salt
+ log.info("Password required, sending challenge: %s" % str(capabilities))
+ packet = ("challenge", self.salt)
+ from xpra.bencode import bencode
+ data = bencode(packet)
+ written = 0
+ while written < len(data):
+ select.select([], [proto._sock], [])
+ written += proto._sock.write(data[written:])
+ return
+
+ passwordFile = open(self.password_file, "rU")
+ password = passwordFile.read()
+ hash = hmac.HMAC(password, self.salt)
+ if client_hash != hash.hexdigest():
+ log.error("Password supplied does not match! dropping the connection.")
+ proto.close()
+ return
+ else:
+ log.info("Password matches!")
+ del capabilities["challenge_response"]
+ self.salt = None #prevent replay attacks
# Okay, things are okay, so let's boot out any existing connection and
# set this as our new one:
if self._protocol is not None:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Parti-discuss mailing list [email protected] http://lists.partiwm.org/cgi-bin/mailman/listinfo/parti-discuss
