SASL' MAX_READ_BYTES is too small

[Prototik]

It's me from #214 again :upside_down_face:

I encountered another issue with cookie auth this time.

Look at this: https://github.com/hypfvieh/dbus-java/blob/9bc3c2cbe95dcf45fd6fdcc6d59822650ebdab8c/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java#L43

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: https://github.com/hypfvieh/dbus-java/blob/9bc3c2cbe95dcf45fd6fdcc6d59822650ebdab8c/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java#L387-L390

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: https://github.com/hypfvieh/dbus-java/blob/9bc3c2cbe95dcf45fd6fdcc6d59822650ebdab8c/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/SASL.java#L99-L107 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