Multiple glitches in administration UI

Hi,

I've upgraded my v0.60 setup today with latest master and begun testing. Found several UI glitches:

When creating a new user, the creation dialog stays once the user is created:

When removing a user, it didn't remove it. Second attempt showed me interal server error. Third attempt worked. Maybe something to do with cache again ?

Extended and Legacy variables are seen as users from burp-ui (also happens for assume_granted variable)

As of today I'm not able to get the ACLs to work.

Example: I have two agents named "burp_protocol1" and "burp_protocol2", and clients for each agents which are called "client.something" and "server.something"

Having created a user with the following permissions:

I am unable to see any agents / clients on the UI. Btw, the JSON blocks agents and clients get inversed by the UI.

I have set assume_granted = false, extended = true and legacy = false.

Assuming the following:

If the rule is a string like 'user1 = desk*', it will match any client that matches 'desk*' no mater what agent it is attached to.", I have removed the agent block from the ACL, and I don't have any better result.

I've tried the user I created the ACL for with a private browser session and logged out / logged in after every change to be sure.

changed the description

Oh, at one time trying to get the ACLs working, I got no clients / agents visible to my user, but could see the backup calendar anyway :)

Hello,

Thanks for trying out those new features.

About the advanced grants that are not working and especially the fact that extended, assume_granted and legacy options appear as users, this is probably because those options have moved to a dedicated ACL section. This is due to the meta_acl engine introduced a few weeks ago.

Here is the new section according to the doc:

# acl engine global options
[ACL]
# Enable extended matching rules (disabled by default)
# If the rule is a string like 'user1 = desk*', it will match any client that
# matches 'desk*' no mater what agent it is attached to.
# If it is a coma separated list of strings like 'user1 = desk*,laptop*' it
# will match the first matching rule no mater what agent it is attached to.
# If it is a dict like:
# user1 = '{"agents": ["srv*", "www*"], "clients": ["desk*", "laptop*"]}'
# It will also validate against the agent name.
extended = false
# If you don't explicitly specify grants, what should we assume?
assume_granted = true
# Enable 'legacy' behavior
# Since v0.6.0, if you don't specify the agents name explicitly, users will be
# granted on every agents where a client matches user's ACL. If you enable the
# 'legacy' behavior, you will need to specify the agents explicitly.
# Note: enabling this option will also disable the extended mode
legacy = false

About the other points, I'll give a look and try to reproduce the errors you encountered.

Yes, I actually read the most recent docs beofre complaining (quite too fast though). I got erraneous because in the current docs [ACL:BASIC] still lists extended, assume_granted, and legacy variables :(

I have moved those three options from BASIC:ACL to ACL section (copy paste from yours here). Trying to setup the following:

[Global]
acl = basic
...

# acl engine global options
[ACL]
# Enable extended matching rules (disabled by default)
# If the rule is a string like 'user1 = desk*', it will match any client that
# matches 'desk*' no mater what agent it is attached to.
# If it is a coma separated list of strings like 'user1 = desk*,laptop*' it
# will match the first matching rule no mater what agent it is attached to.
# If it is a dict like:
# user1 = '{"agents": ["srv*", "www*"], "clients": ["desk*", "laptop*"]}'
# It will also validate against the agent name.
extended = true
# If you don't explicitly specify grants, what should we assume?
assume_granted = false
# Enable 'legacy' behavior
# Since v0.6.0, if you don't specify the agents name explicitly, users will be
# granted on every agents where a client matches user's ACL. If you enable the
# 'legacy' behavior, you will need to specify the agents explicitly.
# Note: enabling this option will also disable the extended mode
legacy = false

[BASIC:ACL]
admin = admin
someuser = '{"agents":["*"],"clients":["*"]}'
+moderator = ""

I still can't see any agent / client with that config. Anything else I might have missed ?

Oops, I need to fix the doc then ;)

These settings look correct, I'll double check and let you know what I find.

Okay, I have identified a first bug...

Here is the patch:

diff --git a/burpui/misc/acl/interface.py b/burpui/misc/acl/interface.py
index ce004bc..9e96378 100644
--- a/burpui/misc/acl/interface.py
+++ b/burpui/misc/acl/interface.py
@@ -182,6 +182,7 @@ class BUIacl(with_metaclass(ABCMeta, object)):
         """
         return False  # pragma: no cover
 
+    @abstractmethod
     def is_server_rw(self, username=None, server=None):
         """:func:`burpui.misc.acl.interface.BUIacl.is_server_rw` tells us
         if a given user has access to a given server in RW mode.
@@ -197,6 +198,7 @@ class BUIacl(with_metaclass(ABCMeta, object)):
         """
         return False  # pragma: no cover
 
+    @abstractmethod
     def is_server_allowed(self, username=None, server=None):
         """:func:`burpui.misc.acl.interface.BUIacl.is_server_allowed` tells us
         if a given user has access to a given server.

Basically, this means the proper is_server_allowed and is_server_rw were never called hence the default False answer was always returned.

I'm still digging for more cases to make sure everything works as expected (I rarely use the multi-agent mode, hence those trivial bugs are not fixed already...) I expect to be able to write a more complete test suite for the next stable release (after v0.6.0) to minimize the manual tests.

Okay, I'll have a quick check with this patch right now. Btw, I guess most of the buttons from the admin interface simply don't call the "back" action.

Example: clicking on "cancel" will stay on the same page

Yes, this button is just a rest form button. I guess I can just remove it since its interest is quite limited (especially on a form containing only one field).

Just checked with the patch, works better (above config works now), but still going a bit crazy.

I've tried to setup the ACLs the way I want them:

myuser = { "agents": ["*"], "clients": ["client*", "serveur*"] }

Once I set up this, I get an Internal Server Error whenever I login as the defined user

Is this the the exact line from your configuration file? If so, you should single-quote the whole ACL. If not, are you able to provide me with the stacktrace?

BTW, I'm available on IRC if you want/need a more real-time chat/troubleshoot (#burp-ui on irc.freenode.net, there is a webclient if you don't have one)

Line is singlequoted (sorry, made a copy paste from GUI and added user info). See you on IRC.

Okay, I have reproduced this error as well.

I have fixed the exception, but the result of this setup is not what we could have expected so I'll need to give it a deeper look.

Actually, with this setup, there is no explicit link between the agents and the clients so the ACL engine does not consider the client* clients to belong to the * agent.

I need to rework a bit the engine to consider this implicit link.

Glad I could challenge your ACL engine :)

Here is a new patch.

I introduced a new option 'implicit_link' which defaults to True in order to implicitly link the agents with the clients. This option is not yet documented because here is just the patch of the meta ACL engine:

diff --git a/burpui/misc/acl/meta.py b/burpui/misc/acl/meta.py
index 480cae2..219096a 100644
--- a/burpui/misc/acl/meta.py
+++ b/burpui/misc/acl/meta.py
@@ -22,6 +22,8 @@ class BUImetaGrant(object):
     def _merge_data(self, d1, d2):
         """Merge data as list or dict recursively avoiding duplicates"""
         if not d1 and not d2:
+            if isinstance(d1, dict) or isinstance(d2, dict):
+                return {}
             return []
         if not d2:
             return d1
@@ -358,7 +360,7 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
         if not username or not client:  # pragma: no cover
             return False
 
-        is_admin = self.is_admin(username)
+        (is_admin, _) = self.is_admin(username)
 
         if self.is_client_allowed(username, client, server):
             # legacy mode: assume rw for everyone
@@ -374,7 +376,7 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
                 server_match = self._server_match(username, server)
 
                 if not server_match and not client_match:
-                    return is_admin or self.opt('assume_granted')
+                    return is_admin or self.opt('assume_granted', True)
 
                 # the whole agent is rw and we did not find explicit entry for
                 # client_match
@@ -406,14 +408,15 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
 
         if self.opt('legacy'):
             return True
-        return is_admin or self.opt('assume_granted')
+        return is_admin or self.opt('assume_granted', True)
 
 
     def is_client_allowed(self, username=None, client=None, server=None):
         """See :func:`burpui.misc.acl.interface.BUIacl.is_client_allowed`"""
         if not username or not client:  # pragma: no cover
             return False
 
-        is_admin = self.is_admin(username)
+
+        (is_admin, _) = self.is_admin(username)
         client_match = self._client_match(username, client)
 
         if not client_match and username == client:
@@ -428,14 +431,19 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
                     return is_admin
 
                 advanced = self._extract_advanced(username)
+                if self.opt('implicit_link', True) and not advanced:
+                    advanced = False
 
-                if not client_match and server_match not in advanced and \
+                if advanced is not False and not client_match and \
+                        server_match not in advanced and \
                         (server_match in self._extract_advanced_mode(username, 'ro', 'agents') or
                          server_match in self._extract_advanced_mode(username, 'rw', 'agents')):
                     return True
 
-                advanced = advanced.get(server_match, advanced.get(server, []))
-                if client_match not in advanced and client not in advanced:
+                if advanced is not False:
+                    advanced = advanced.get(server_match, advanced.get(server, []))
+                if advanced is not False and client_match not in advanced and \
+                        client not in advanced:
                     return is_admin
 
         return client_match is not False or is_admin
@@ -445,11 +453,11 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
         if not username or not server:  # pragma: no cover
             return False
 
-        is_admin = self.is_admin(username)
+        (is_admin, _) = self.is_admin(username)
         if self.is_server_allowed(username, server):
             server_match = self._server_match(username, server)
             if not server_match:
-                return self.is_admin or self.opt('assume_granted')
+                return is_admin or self.opt('assume_granted', True)
 
             advanced = self._extract_advanced(username)
 
@@ -458,7 +466,7 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
 
         if self.opt('legacy'):
             return True
-        return is_admin or self.opt('assume_granted')
+        return is_admin or self.opt('assume_granted', True)
 
     def is_server_allowed(self, username=None, server=None):
         """See :func:`burpui.misc.acl.interface.BUIacl.is_server_allowed`"""
@@ -466,7 +474,7 @@ class BUIgrantHandler(BUImetaGrant, BUIacl):
             return False
 
         server_match = self._server_match(username, server)
-        is_admin = self.is_admin(username)
+        (is_admin, _) = self.is_admin(username)
 
         if server_match is None and self.opt('legacy'):
             server_match = False

Applied the patch, getting better, but I am still not able to find a configuration that fits what I expect.

Here's what I am trying to do:

I have 2 agents, with each multiple clients like:

• bui.something
• client.something
• server.something

I want to create a user that can create new clients on each agent, do restore operations and deletions (actually an operational administrator, which can do everything except configuring burp server settings itself). So I created the user with the following rights:

myadmin = '{"agents":["*"],"clients":["client*","server*"]}'

So far so good, I had all client.* and server.* clients showing, without restore / config permissions. I then tried to fine tune rights into:

myadmin = '{"agents":{"ro":["*"]},"clients":["client*","serveur*"]}'

With this setup, only my bui.* clients show which obviously shouldn't.

I then tried the following:

myadmin = '{"agents":{"ro":["*"]},"clients":{"rw":["client*","server*"]}}'

which gave me the same result, only my bui.* clients show.

When I added myadmin account to the moderator group, I still couldn't see the clients, but could edit their configuration from the agent config page...

Back to myadmin = '{"agents":{"ro":["*"]},"clients":["client*","server*"]}', I have made a voluntary misspelling from 'server' to 'serveur', which showed me another problem:

Once I corrected the misspelling, I got some strange behavior on the server list: Agent1 has 7 clients, of which 1 bui.something, 2 client.something and 4 server.something Agent2 has 2 clientn, of which 1 bui.something, 1 server.something

The server list shows that Agent1 has 2 clients (which is correct due to the fact that I mispelled server). The server list shows that Agent2 has 0 clients, but I still can click on it and see server.something client. This behaviour came back to normal once I logged out / logged in, but what bugs me here is that half of the info was correct, and half wasn't... Maybe a drop cache on configuration changes would be a good idea ?

In the end, I also ran into various intermittent Server Internal Errors (real ones, not python catched ones), which resolved by themselves after waiting a minute. I still have to investigate this one though.

I just tried another thing, it seems that as soon as I use the "ro" or "rw" keywords, the ACLs play crazy.

myadmin = '{"agents":["*"],"clients":["client*","server*"]}' shows my clients.

myadmin = '{"agents":["*"],"clients":{"rw":["client*","server*"]}}' shows no clients.

I've been running some tests as well on my side and I'm currently reworking a bit the engine.

The complexity lies in the fact that several syntax are allowed as ACL rules. I may end-up allowing only one syntax to simplify the engine logic... I don't know yet.

Anyway, the more explicit you are, the easiest it is for the engine to understand what you really want.

Exemple:

myadmin = '{"agents": {"someagent": {"ro": ["client1", "client2"], "rw": ["client3"]}, "*": {"ro": ["*"]}}}'

This way, there are explicit links between the agents and the clients.

Also, only moderators (or admins) can create new clients if they have rw rights on the agent. But again, this part was not deeply tested/documented so you may encounter unexpected behaviors.

Sorry for all those glitchs...