SASL' MAX_READ_BYTES is too small

[Prototik]

It's me from #214 again 🙃

I encountered another issue with cookie auth this time.

Look at this:

dbus-java/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java

Line 43 in 9bc3c2c

private static final int MAX_READ_BYTES = 64;

So dbus-java would read at most 64 bytes for each line at auth stage, but let's do some math for cookie auth:
-5 bytes for DATA prefix, leftover is hexencoded, so divide by 2 with rounding down = 29 bytes of actual data at max.
Cookie auth data consist of 3 parts: cookie context, cookie id and random part, space delimeted, so -2 bytes = 27.
Default cookie context is org_freedesktop_java, which is 20 bytes, leftover is 7 bytes.
Default cookie id is milliseconds since unix epoch, which is 12 bytes, leftover is... -5 bytes.
Default random part is 8 bytes long, so -13 bytes.

But that's only true for default settings of dbus-java, other implementation can use even longer parts, but as you can see even with that cookie auth is broken

[hypfvieh]

I increased the read size to 1 MByte, this should be enough for everything.
If not, something is really broken or behaves bad.

[Prototik]

I believe another issue with cookie here:

dbus-java/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java

Lines 387 to 390 in 9bc3c2c

	logger.debug("Sending challenge: {} {} {}", context, id, challenge);
	_c.setResponse(stupidlyEncode(context + ' ' + id + ' ' + challenge));
	return SaslResult.OK;

Here is challenge string generated, but result of the operation is set to OK, which means 'you authorized, go ahead and do what you want'. Instead it should be CONTINUE to send challenge string to the client... Can you confirm that?

Retrieving cookie from keyring is also buggy:

dbus-java/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java

Lines 99 to 107 in 9bc3c2c

	TimeMeasure tm = new TimeMeasure();
	while (null != (s = r.readLine())) {
	String[] line = s.split(" ");
	long timestamp = Long.parseLong(line[1]);
	if (line[0].equals(_id) && !(timestamp < 0 || (tm.getElapsedSeconds() + MAX_TIME_TRAVEL_SECONDS) < timestamp || tm.getElapsedSeconds() - EXPIRE_KEYS_TIMEOUT_SECONDS > timestamp)) {
	lCookie = line[2];
	break;
	}
	}

Both cookie filter clauses (tm.getElapsedSeconds() + MAX_TIME_TRAVEL_SECONDS) < timestamp and tm.getElapsedSeconds() - EXPIRE_KEYS_TIMEOUT_SECONDS > timestamp) contains tm.getElapsedSeconds(), which is seconds since creating of TimeMeasure object, so basically always is 0, and timestamp is seconds since unix epoch, which is always waaaay greater than zero, so comparing then is useless and not correct.

[Prototik]

This patch seems work for me: cookie auth works perfectly for both client & server:

---
 .../org/freedesktop/dbus/connections/SASL.java | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
index 190cf2c..89d2d1e 100644
--- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
+++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
@@ -93,15 +93,23 @@ public class SASL {
         }
 
         File f = new File(keyringDir, _context);
+        long currentTime = System.currentTimeMillis() / 1000;
         try (BufferedReader r = new BufferedReader(new InputStreamReader(new FileInputStream(f)))) {
             String s = null;
             String lCookie = null;
 
-            TimeMeasure tm = new TimeMeasure();
             while (null != (s = r.readLine())) {
                 String[] line = s.split(" ");
-                long timestamp = Long.parseLong(line[1]);
-                if (line[0].equals(_id) && !(timestamp < 0 || (tm.getElapsedSeconds() + MAX_TIME_TRAVEL_SECONDS) < timestamp || tm.getElapsedSeconds() - EXPIRE_KEYS_TIMEOUT_SECONDS > timestamp)) {
+                if (line.length != 3) {
+                    continue;
+                }
+                long timestamp;
+                try {
+                    timestamp = Long.parseLong(line[1]);
+                } catch (NumberFormatException _ex) {
+                    continue;
+                }
+                if (line[0].equals(_id) && timestamp >= 0 && currentTime >= timestamp - EXPIRE_KEYS_TIMEOUT_SECONDS && currentTime < timestamp + MAX_TIME_TRAVEL_SECONDS) {
                     lCookie = line[2];
                     break;
                 }
@@ -388,7 +396,7 @@ public class SASL {
                     logger.debug("Sending challenge: {} {} {}", context, id, challenge);
 
                     _c.setResponse(stupidlyEncode(context + ' ' + id + ' ' + challenge));
-                    return SaslResult.OK;
+                    return SaslResult.CONTINUE;
                 default:
                     return SaslResult.ERROR;
                 }
@@ -555,7 +563,7 @@ public class SASL {
                     switch (c.getCommand()) {
                     case OK:
                         send(_sock, BEGIN);
-                        state = SaslAuthState.AUTHENTICATED;
+                        state = SaslAuthState.FINISHED;
                         break;
                     case ERROR:
                     case DATA:
-- 
2.41.0

[Prototik]

btw anonymous auth doesn't work with zbus as it wants reply to that empty DATA packet, this is also a fix for it:

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
index 89d2d1e..166c67f 100644
--- a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
+++ b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java
@@ -352,6 +352,10 @@ public class SASL {
             response = stupidlyEncode(buf);
             _c.setResponse(stupidlyEncode(clientchallenge + " " + response));
             return SaslResult.OK;
+        case AUTH_ANON:
+            // Pong back DATA if server wants it for anonymous auth
+            _c.setResponse(_c.getData() == null ? "" : _c.getData());
+            return SaslResult.OK;
         default:
             logger.debug("Not DBUS_COOKIE_SHA1 authtype.");
             return SaslResult.ERROR;

[Prototik]

@hypfvieh Should I format these patches as pull request or it's fine for you to apply it manually?

[hypfvieh]

PRs are always welcome