Ticket #501 (closed Bug report: fixed)

Opened 10 years ago

Last modified 9 years ago

PATCH: Bug in fw_iptables.c (version 1.1.5)

Reported by: Piero Owned by: gbastien
Priority: blocker Milestone: Gateway 1.1.5
Component: Gateway Version:
Keywords: Cc:

Description

Finally I understand why the gateway leaves a user to be connected while it should be logged off.

If the user client submits a valid token the gateway (fw_iptables.c) executes:

iptables -t mangle -A WiFiDog_Outgoing -s 10.4.19.235 -m mac --mac-source 00:14:BF:D1:XX:XX -j MARK --set-mark 2

and this is fine.

But if the user is still allowed and submits the token again, the gateway just executes the command again, so when the auth server denies the connection to the client, and the gateway executes:

iptables -t mangle -D WiFiDog_Outgoing -s 10.4.19.235 -m mac --mac-source 00:14:BF:D1:XX:XX -j MARK --set-mark 2
 iptables -t mangle -D WiFiDog_Incoming -d 10.4.19.235 -j ACCEPT

this is just unusefull since the previous command is excecuted twice.

So the gateway just deletes the ip from the client list and looses control over the client who is still able to pass the firewall.

Workaround:

As I noticed in the debug messages of the gateway, if the client still uses the service, but is not on the client list, a debug message is shown:

[3][Sat Jan  1 00:43:48 2000][2297](fw_iptables.c:556) Could not find 10.4.19.235 in client list

this means that the gateway feels that something is going wrong, so why dont just block that IP instead of just showing the debug message?

I wish I could do it myself but I think it is something that should be fixed in the official version, and I cant fix myself.

The error is on source code fw_iptables.c on lines 519 and 556, actually the code is:

pclose(output);

Please fix this, or the all great system would be unusefull in a commercial environment.

Greets. Piero

Attachments

patch_501_benoitg_2010_03_02.patch Download (3.0 KB) - added by benoitg 9 years ago.

Change History

Changed 10 years ago by piero

of course the word "but" should be "bug", but I guess it was understandable :-)

Changed 10 years ago by piero <piero@…>

no one thinks this is a real bug? It invalidates any security process, so to make the platform unusable in commercial environments

Changed 10 years ago by macjones

Yes this bug is real and confirmed, with any sort of voucher based authentication, you can simply hit back on your browser, resubmit the voucher and you're logged in forever. If we can not get the code in fw_iptables.c, then your auth system for vouchers must record who has had auth:1 and not allow a voucher to be revalidated as auth:1.

Changed 10 years ago by Mac Jones

fixed the bug...

edit fw_iptables.c and add the line

iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_OUTGOING, ip);

this will delete all mention of the firewall rule for the ip address that could not be found in the client list.

here is a copy of the lines found towards the end of fw_iptables.c


                    
            } else {
                debug(LOG_ERR, "Could not find %s in client list", ip);
		iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_OUTGOING, ip);
		debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip,TABLE_WIFIDOG_OUTGOING);
            }
	    UNLOCK_CLIENT_LIST();
        }
    }
    pclose(output);

yes it would be tidier to not add it twice in auth.c maybe we'll get to that one day, for now this seems to work OK, thanks to my wife Julie for figuring this out..

Changed 10 years ago by anonymous

It's so good we solved this issue, and I believe this is the right way. Not validating users that already have auth:1 is unusefull. Actually the auth server could have set the auth:1 for a user but this information could have been lost in the access point, so the user wouldn't be able to get the auth:1 back from the server.

Could anyone please update this in the official version? I have trouble compiling the software. Thank You.

Changed 10 years ago by macjones

may as well delete incoming AND outgoing rules orphans.

 } else {
                debug(LOG_ERR, "Could not find %s in client list", ip);
                iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_OUTGOING, ip);
                debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_OUTGOING);
                iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_INCOMING, ip);
                debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_INCOMING);
            }
            UNLOCK_CLIENT_LIST();
        }
    }
    pclose(output);

Changed 10 years ago by daniel@…

Has anyone built a version of the gateway for Kamikaze with the above fixes? Would appreciate an email with the binary if so!

Changed 10 years ago by Mac Jones

Patch file..


diff -crB wifidog-1.1.5/src/fw_iptables.c wifidog-1.1.5_netstop/src/fw_iptables.c *** wifidog-1.1.5/src/fw_iptables.c 2007-11-02 09:04:20.000000000 +1300 --- wifidog-1.1.5_netstop/src/fw_iptables.c 2009-03-27 15:49:24.607533755 +1300 *************** *** 517,522 **** --- 517,526 ----

}

} else {

debug(LOG_ERR, "Could not find %s in client list", ip);

+ iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_OUTGOING, ip); + debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_OUTGOING); + iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_INCOMING, ip); + debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_INCOMING);

} UNLOCK_CLIENT_LIST();

}


Changed 10 years ago by Mac Jones

again....

diff -crB wifidog-1.1.5/src/fw_iptables.c wifidog-1.1.5_netstop/src/fw_iptables.c
*** wifidog-1.1.5/src/fw_iptables.c	2007-11-02 09:04:20.000000000 +1300
--- wifidog-1.1.5_netstop/src/fw_iptables.c	2009-03-27 15:49:24.607533755 +1300
***************
*** 517,522 ****
--- 517,526 ----
                  }
              } else {
                  debug(LOG_ERR, "Could not find %s in client list", ip);
+ 		iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_OUTGOING, ip);
+ 		debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_OUTGOING);
+ 		iptables_fw_destroy_mention("mangle", TABLE_WIFIDOG_INCOMING, ip);
+ 		debug(LOG_ERR, "Deleted firewall rule for %s in table %s", ip, TABLE_WIFIDOG_INCOMING);
              }
  	    UNLOCK_CLIENT_LIST();
          }

Changed 10 years ago by macjonesnz@…

Binary sent....if anyone else needs it, email me.

Changed 10 years ago by benoitg

  • owner set to benoitg
  • priority changed from normal to blocker
  • status changed from new to assigned
  • summary changed from there is a but in fw_iptables.c (version 1.1.5) to Bug in fw_iptables.c (version 1.1.5), patch included

Changed 10 years ago by benoitg

  • summary changed from Bug in fw_iptables.c (version 1.1.5), patch included to PATCH: Bug in fw_iptables.c (version 1.1.5)

Changed 9 years ago by gbastien

Benoît, do you want me to commit this one?

Changed 9 years ago by benoitg

Changed 9 years ago by benoitg

  • owner changed from benoitg to gbastien
  • status changed from assigned to new

I was a bit weary of this patch, as that situation should never happen. And like many other things it just fell off my TODO list.

I think I found and patched the real source of the problem: the gateway didn't even try to clear the firewall on deny.

I think you can commit this newly attached patch (untested).

I left the workaround, with more explicit error messages, but we'll have to test and watch this closely.

Changed 9 years ago by gbastien

  • status changed from new to assigned

It actually does happen and is quite easy to reproduce, though I couldn't say exactly how... But a few consecutive logins with same username and playing around the login page usually does it.

I'll commit your patch and keep investiguating why that which should never happen happens...

Changed 9 years ago by benoitg

I KNOW it happens ;) My problem was that the initial patch was a band-aid that basically garbage collected unknown firewall entries if there was a counter update for someone not in the client list. So the patch does two things:

* I fixes the root cause (I think, but I am not sure) * Makes the band-aid yell louder

That way: 1-We don't have to look too hard if side effects do turn up. 2-If the patch fixes the problem, and the band-aid doesn't yell, we know I found the right cause.

Changed 9 years ago by gbastien

  • status changed from assigned to closed
  • resolution set to fixed

Fixed in [1454]

I meant to say the band-aid still yells, but I spoke too soon, the fault came from my auth-server under test who misbehaved ;-)

The patch was committed and will be part of the next official release by the end of March, and I'll keep scrutinizing it to see if you found the root cause or not. Thanks!

Note: See TracTickets for help on using tickets.