solutions-nitriques / anti_brute_force Goto Github PK

You may know that I released the Members Anti Brute Force extension which extends the functionality of Anti Brute Force to Members login and password reset.

At the moment my extension registers failures, but doesn't do anything with colored lists. I have two choices to make it work:

1. Rebuild authorLoginFailure logic in my extension.
2. Build a function to get the ABF driver and then do s.th. like $this->getABFDriver()->authorLoginFailure($context).

I would prefer the second solution, but unfortunately failures will be registered with the name of your extension, not mine. To overcome this, what would you think about a small change to your extension's authorLoginFailure function which allows to pass a name that should be registered? Like so:

public function authorLoginFailure($context, $ext_name = self::EXT_NAME) {
    // do not do anything is ip is white listed
    if (!ABF::instance()->isWhiteListed()) {

        // register failure in DB
        ABF::instance()->registerFailure($context['username'], $ext_name);

        // if user is now banned
        if (ABF::instance()->isCurrentlyBanned()) {
            // register into gray list
            ABF::instance()->registerToGrayList($ext_name);
            // move to black list if necessary
            ABF::instance()->moveGrayToBlack($ext_name);
        }
    }
}

Do you think that is too much of a hack?

I think you can remove this dependency and call the database with Symphony::Database. ASDC doesn't really add anything extra except returning objects rather than arrays, so I'm not sure you'll see any benefit. Besides, ASDC is no longer maintained and is likely to be deprecated in the future anyway :-)

I am running a MySQL replicate, which (by design of the MySQL binlog, obviously) completely breaks on database errors — those errors may be less critical when Symphony deals with the main database.

I noticed a problem with the registerFailure function. My replicate breaks with the following error as soon as failures are actually registered (private data are masked by xxx):

Last_SQL_Error: Error 'Duplicate entry '93.xxx.xxx.xxx' for key 'PRIMARY'' on query. Default database: 'symphony'. Query: 'INSERT INTO sym_anti_brute_force
    (`IP`, `LastAttempt`, `Username`, `FailedCount`, `UA`, `Source`, `Hash`)
    VALUES
    ('93.xxx.xxx.xxx', NOW(),        'xxxxxxxxxx', 1,            '','Anti Brute Force', UUID())'

So it looks as if the check you implemented to decide whether to UPDATE or INSERT does not work as expected.

Wouldn't it be cooler anyway to either use ON DUPLICATE KEY UPDATE or maybe Symphony's database insert method? (Im am not sure about the latter, I haven't checked if it will it work properly with custom tables or not.)

In this line inline event handlers are added.

However, inline event handlers should be avoided.

(I wouldn't care removing these buttons altogether, because you can as well use the menu to navigate between lists. I always saw the buttons as a "superfluous second way to do something".)

I don't know why, but using the latest code from the dev branch, Symphony thinks that this extension requires Symphony 2.5.

In my environment, the user agent is always empty. I looked at the code and wondered why you use $_ENV["HTTP_USER_AGENT"] here. Is the user agent supposed to be in $_ENV at all?

If I change it to $_SERVER["HTTP_USER_AGENT"], it works as expected.

Now that Symphony 2.6 is out for a while, we should have a version of the extension that is "officially" compatible.

The FailedLoginAttempt is fired BEFORE the AdminPagePreGenerate

Right now, if a blocked user continue to POST login data to the login page it's stils tries to auth the user, register a failed attemp (which reset the block duration), and then throws the exception.

How about fixing this? preventing Symphony to try to auth the user ?

Since this is a _open-source security_ piece of software, I feel I have to publish the issues I know about !

Everything that is NOT UI related... keep UI in extension class

With the help of a special page, user can get a unban link via email.
This feature can be disabled via the preferences pages

Using Symphony 2.5.0beta2, unbanning via email seems broken.

There was a problem locating your account. Please check that you are using the correct email address.

And yes, I double-checked that I used my account's email address. :-)

Even if auto-unban is off, I find the unban via email link in the error message, and the procedure is working (apart from the small issue #21). So maybe this flag doesn't do anything at all?

No matter what I type into the input field, my own IP gets added to the colored list.

On the "Banned IPs" page, if I select an entry in the index table and delete it using the "with selected" menu, the table row is still visible afterwards, although it has already been deleted fron the database table. (It will be gone when the page is loaded the next time, of course.)

• Blacklist -> Banned forever, overrides all others tables, NO EMAIL UN BAN POSSIBLE
• Whitelist -> Never gets banned
• Greylist -> Banned users by ip will go automatically into greylist for x days/months. If IP is greylisted more than y times in x month/days -> BLACKLIST THEM... for ever... or until admin wants too.

Should only limited to 1 record per IP.

See Issue #3 for the full discussion

When IPv6 addresses pass through to the ABF extension, you will see a database error on the second login attempt, onviously caused by the fact that the IP address gets cut off in the IP column (defined as VARCHAR 16).

Symphony Fatal Database Error: Duplicate entry '2003:63:2a15:376' for key 'PRIMARY'

An error occurred while attempting to execute the following query

INSERT INTO sym_anti_brute_force ( IP, RawIP, LastAttempt, Username, FailedCount, UA, Source, Hash ) VALUES ( '2003:63:2a15:3767:358f:480e:202d:6883', '2003:63:2a15:3767:358f:480e:202d:1234', NOW(), 'michael-e', 1, 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.2.14 (KHTML, like Gecko) Version/10.0.1 Safari/602.2.14', 'Anti Brute Force', UUID() )

The issue will not occur if you increase the length of the IP columns (in all four tables) to VARCHAR 45 (whis is the maximum lenght of an IPv6 address, see http://stackoverflow.com/a/7477384).

There is a typo on line 114 of class.ABF.php. I noticed it on screenshot and then checked if it still is in code :).

many worng usernames

should probably be:

_many _wrong* usernames*

On Symphony's preferences page:

• there is too much padding between the "lines" (compare it to the SMTP settings!)
• the checkboxes are preceded by the label — it should be the other way round

Well, you get the idea...

Maybe we should add the following facts to the README?

If you are using Symphony on a server that sits behind a proxy, it will always track 127.0.0.1 as remote address, simply because PHP doesn't see anything else in $_SERVER['REMOTE_ADDR']. This is, of course, really bad.

Fortunately the extension already offers a simple mechanism to fix this problem. In /manifest/config.php, you can add an additional setting:

'remote-addr-key' => 'HTTP_X_FORWARDED_FOR',

(In most cases the name of the header provided by the proxy will be HTTP_X_FORWARDED_FOR, but this doesn't need to be the case. A badly configured proxy may not add an additional header at all. So you must do some research if it doesn't work.)

Hi Nicolas,
Is possible to hide System > ABF menu itens for Authors, shows only for Admins?

I add 127.0.0.1 for White list and try more than 5 times force error pass. Ok, don't block my ip.
But when I try correct pass, page refresh and can't login.

The inconsistent spelling of the word "grey" (or "gray")—English vs. American spelling— is the reason for at least one bug: The menu item "Gray listed IPs" doesn't work, because the value of the GET param is "grey" instead of "gray".

Shouldn't be too hard to fix.

The callback function name initaliseAdminPageHead is a typo. The function is public, so correcting the typo would require releasing a new major version (2.0). Nevertheless I suggest to do it (because I don't expect that any other extension ever called this public function).

If you like, I can send a PR for this after #33 has been pulled.

Re #19

It does not contain a second parameter anymore. (2.6)

Maybe use a default value ?

New signature:

public function update($previousVersion = false)

Is the newly added legend "Use this form to manually add an IP." really necessary in your eyes? It pushes the input to the right (in Safari), which doesn't look very elegant. I'd rather live without the legend, because the form is rather self-explaining.

As you can see in the screenshot (I used a small window to make it more obvious), also the alignment between "your" input and the "with selected" menu is not perfect.

ABF::isIPValid will return false for IPv6 addresses. Therefore, it is not possible to add an IPv6 address to the whitelist. (There may be other negative effects as well.)