Buffer overflow in password when many special chars

[disk91]

I've been facing an issue with a long password (256 char) containing many special chars.
Apparently the HTTP request is truncated when the url encoded password is too large..
Here is the debug log with a bit shorter password

DEBUG:  http_send:
POST /remote/logincheck HTTP/1.1
Host: XXXXX:XXXX
User-Agent: Mozilla/5.0 SV1
Accept: */*
Accept-Encoding: gzip, deflate, br
Pragma: no-cache
Cache-Control: no-store, no-cache, must-revalidate
If-Modified-Since: Sat, 1 Jan 2000 00:00:00 GMT
Content-Type: application/x-www-form-urlencoded
Cookie: 
Content-Length: 243

username=xxxxx&credential=********************************************************************************************************************Cr%60LK%25Gj%3F%24jFiO_1%23%7D%5EBgg%23%60~%3Fa%2F%298%3FaEL%5Boy%289_%23sM%22qcg%27&realm=&ajax=1

Here is the result with a full length password:

DEBUG:  http_send:
oy%289_%23sM%22qcg%27

Changing the password for the same length with less special characters works.
Official Forti client works with initial password / login on the webpage is also working.

[DimitriPapadopoulos]

Which version of openfortivpn?

[disk91]

1.15.0

[DimitriPapadopoulos]

The length of the password buffer when read from stdin - PWD_BUFSIZ - is 4096. The length of the password when read as an openfortivpn command line option or from the configuration files does not seem to be limited. The password seems to be read correctly as far as I can see.

The next question is how the password is sent to the FortiGate. I understand oy%289_%23sM%22qcg%27 (URL-decoded as oy(9_#sM"qcg') shows the truncated password, doesn't it? This is suspect of course, but the real stuff happens in auth_log_in():
https://github.com/adrienverge/openfortivpn/blob/73075e5/src/http.c#L627-L783
This might be an issue with url_encode():
https://github.com/adrienverge/openfortivpn/blob/73075e5/src/http.c#L659
See for example discussion about URL encoding in #351. Could you perhaps add a couple printf() calls after this url_encode() call and print tunnel->config->password and password? Just to make sure tunnel->config->password is the expected password and that is it properly URL-encoded into password?

Ah, I may have something here:
https://github.com/adrienverge/openfortivpn/blob/73075e5/src/http.c#L630-L632
The size of the username, password and realm fields in struct vpn_config used to be FIELD_SIZE, but #361 changed to a dynamically allocated password field. Unfortunately this part of the code where we URL-encode the password had not been updated accordingly. Are your passwords larger than 64 characters? The result here is that the dynamically allocated tunnel->config->password is URL-encoded into the fixed size password, which of course results in buffer overrun and memory corruption. I'm surprised tools such as Coverity Scan haven't caught this error.

I recommend we revert #361 and choose a static but decent size for passwords - such as 4096. How does a password maximum length of 4096 sound?

[DimitriPapadopoulos]

The password maximum length might be 128, at least in some cases:
Password policy

[...] the FortiGate unit requires only that passwords be at least eight characters in length, but up to 128 characters is permitted.

I haven't ever configured a FortiGate, so I have no idea when and how this limit is applied. It might not apply if the password is defined elsewhere, for example in an ActiveDirectory domain.

@disk91 @mrbaseman Any clue about the maximum permitted size of passwords?

[DimitriPapadopoulos]

The 128 maximum size may apply to admin passwords only:
Technical Tip: FortiOS / FortiGate objects naming rules

[DimitriPapadopoulos]

On the other hand RADIUS/LDAP server domain name are limited to 63 characters:
Technical Tip: FortiOS / FortiGate objects naming rules

Therefore, I will limit the size of realm to 63 instead of FIELD_SIZE.

[DimitriPapadopoulos]

The logon name - sAMAccountName attribute - is limited to 20 in Active Directory:
SAM-Account-Name

Recent versions of Active Directory can use unlimited logon names (using the userPrincipalName attribute):
User-Principal-Name attribute

On the other hand the Windows Username itself is limited to 256 characters:
Username

I believe we can keep the current value of FIELD_SIZE (64) for username`.

[DimitriPapadopoulos]

The maximum password length in Active Directory appears to be 256 - even though the GUI for end-users shows only 127 of the characters!
Password length best practices

I suggest we limit the size of password to 256 as it "ought to be enough for anybody".