You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by st...@lycos.co.uk on 2005/05/02 14:47:17 UTC
Re: SSL error trapping
I got it working !
Here is my solution. Questions 1 & 4 still remain.
The additional module (ssl_error) also works, although I need to fine-tune it.
Feel free to criticize and enhance.
Marc
-------------------
In 'ssl_hook_Access( )' (ssl_engine_kernel.c), the last line can be replaced by:
{ /* MSTERN: SSL errors trapping */
const char *ssl_client_verify = ssl_var_lookup( r->pool, r->server, r->connection, r, apr_pstrdup(r->pool, "SSL_CLIENT_VERIFY") );
if ( ! ssl_client_verify ) {
ap_log_error(APLOG_MARK, APLOG_NOERRNO | APLOG_DEBUG, 0, r->server, "Unable to get SSL Verification STATUS");
return HTTP_INTERNAL_SERVER_ERROR; /* cannot find ssl_var_lookup( ) */
}
if ( ! *ssl_client_verify ) return HTTP_INTERNAL_SERVER_ERROR; /* not in a SSL session */
if ( strcmp(ssl_client_verify, "NONE") == 0 ) return HTTP_INTERNAL_SERVER_ERROR; /* SSL negociation not finished */
ap_log_error(APLOG_MARK, APLOG_NOERRNO | APLOG_DEBUG, 0, r->server, "Certificate Verification Status: %s", ssl_client_verify);
if ( strcmp(ssl_client_verify, "SUCCESS") == 0 ) return DECLINED;
if ( strncmp(ssl_client_verify, "GENEROUS", 8) == 0 ) return DECLINED;
}
apr_table_setn(r->notes, "ssl-access-forbidden", "1");
{ /* Check if mod_ssl_error is loaded */
extern AP_DECLARE_DATA module *ap_top_module;
module *m;
for ( m = ap_top_module; m; m = m->next )
if ( strcmp(m->name, "mod_ssl_error.c") == 0 ) return DECLINED;
}
return HTTP_FORBIDDEN;
----- Original Message -----
From: sternmarc@lycos.co.uk
To: dev@httpd.apache.org
Sent: Friday, April 29, 2005 12:26 PM
Subject: SSL error trapping
In case a SSL connection fails because a certificate is expired, or a CRL is unavailable, etc., the browser receives a SSL error that results in a cryptic technical error displayed to the user - sometimes only an error number like in Firefox. In such a situation, the SSL connection could be established, and a HTTP_FORBIDDEN (403) error returned. By adding another module, It is even possible to trap the exact SSL error and redirect to a page with the specific error message ("Your certificate is expired", "We cannot check the validity of the certificate - retry later", ...).
I plan to add all of this, but I'd like to check with everybody the best design and implementation.
Technical description (based on 2.0.54):
In ssl_io_filter_connect( (ssl_engine_io.c), we have 2 cases (at line 1147 and 1173) where the connection may break because of certificates verification/validation problem: ' return ssl_filter_io_shutdown(filter_ctx, c, 1); '
If we do not return, we can trap the error in 'ssl_hook_Access( )' (ssl_engine_kernel.c).
At the end, instead of returning DECLINED, we have to check the certification verification result, then return either DECLINED or HTTP_FORBIDDEN.
This is quite generic; then we may use another module to make redirections depending on the exact error.
Questions and choices:
1. Could this be accepted as a standard feature (thus for everybody), or should I use
- a compilation flag
- a run-time directive
2. Does the other module trapping the 'hook_access' receive the control in case of the previous module returns a HTTP_FORBIDDEN error ?
If not, we could detect at run-time that the trapping module is loaded (how exactly ?), and, in this case, return DECLINED.
3. To check the certification verification process, I can use the string "SSL_CLIENT_VERIFY", but isn't there any real error code (int) available ? It would be cleaner to use the exact OpenSSL error codes than a string. I cannot find this code, even inside 'ssl_hook_Access( )' in ssl_engine_kernel.c. Awk ...
4. Should this trapping be extended to other non fatal SSL errors ? Is it also possible to trap fatal errors and redirect to HTTP ?
If you have other remarks or ideas, please tell me.
Marc
Buffer overrun in modssl
Posted by st...@lycos.co.uk.
I think I found a buffer overrun in ssl_callback_SSLVerify_CRL( ) (ssl_engine_kernel.c):
char buff[512]; /* should be plenty */
[...]
n = BIO_read(bio, buff, sizeof(buff));
buff[n] = '\0';
If there are more than 512 bytes, n=512, thus we write in buff[512].
We should use
n = BIO_read(bio, buff, sizeof(buff) - 1);
Am I right ?
Marc
Error in BOOL definition ?
Posted by st...@lycos.co.uk.
In mod_ssl.h, BOOL is defined as "unsigned int".
Some mod_ssl parameters are defined as BOOL:
struct SSLSrvConfigRec {
SSLModConfigRec *mc;
BOOL enabled;
BOOL proxy_enabled;
const char *vhost_id;
int vhost_id_len;
int session_cache_timeout;
modssl_ctx_t *server;
modssl_ctx_t *proxy;
};
In ssl_config.c, the parameters are initialized with UNSET:
sc->enabled = UNSET;
sc->proxy_enabled = UNSET;
UNSET is defined as -1 ==> signed/unsigned problem
Shouldn't we change the BOOL definition to signed int ?
Rem: On some compilers, BOOL may be already defined, so the run-time libraries definition is used instead of mod_ssl one
Re: Easy fixes
Posted by Nick Kew <ni...@webthing.com>.
Joshua Slive wrote:
>
> On Tue, 31 May 2005 sternmarc@lycos.co.uk wrote:
>
>> Couldn't we add one (or several) keywords for important problems that
>> have a trivial solution ? This would allow quicker integration.
>
>
> I don't think it would help, really. It still requires people to go
> searching back through the bug database, which few developers are
> willing to do.
I don't think that's the whole story. I have trawled it a number of
times for bugs reported in a particular module (or subsystem, in the
case of proxy) I've been working on, and I'm sure others do too.
Fixing bugs under HEAD/CTR is easy. Backporting to 2.0/RTC is the
bottleneck: we have a lot of fixes that should have been backported
but haven't. It might be beneficial to have a relaxation of the
policy for dealing with fixes that are sufficiently small/simple and
which address nontrivial bugs.
> I suggest you simply catalogue the bugs you think qualify and post to
> the list with 1) a link to the bug report; 2) a short description; and
> 3) why you think it is easy/important to fix.
Not to mention PatchAvailable :-)
> Bringing things to the attention of the mailing list (repeatedly, if
> necessary) is a more effective way to get them fixed than to add more
> complexity to bugzilla.
I guess that's right in practice. But banging on about something when
noone is paying attention is at best difficult and demotivating. Come
to think of it, before I got commit, I got better results by bugging
people on IRC than by posting to the list.
--
Nick Kew
Re: Easy fixes
Posted by Joshua Slive <jo...@slive.ca>.
On Tue, 31 May 2005 sternmarc@lycos.co.uk wrote:
> Couldn't we add one (or several) keywords for important problems that
> have a trivial solution ? This would allow quicker integration.
I don't think it would help, really. It still requires people to go
searching back through the bug database, which few developers are willing
to do.
I suggest you simply catalogue the bugs you think qualify and post to the
list with 1) a link to the bug report; 2) a short description; and 3) why
you think it is easy/important to fix.
Bringing things to the attention of the mailing list (repeatedly, if
necessary) is a more effective way to get them fixed than to add more
complexity to bugzilla.
Joshua.
Easy fixes
Posted by st...@lycos.co.uk.
Couldn't we add one (or several) keywords for important problems that have a trivial solution ?
This would allow quicker integration.
I think typically about memory leaks and buffer overruns. Once they are discovered (which is the real difficult part), the fix is usually trivial - often one line. The verification of the fix is thus very easy.
This kind of bug could be marked as "quick win" and included quicker than solutions that need a lot of verification and testing. I see memory leak problems floating from 1.5 year, and still in NEW state (ex: #25659, #25667). That's a pity.
Obviously, there may be a better solution than the keyword - the point is to notify that the corretion is a "quick win".
"quick win" may have to be defined, like
- category = memory leaks, buffer overruns, invalid pointers, invalid cast, etc.
- max. lines/instructions (without declarations): 5 / 10 ?
- no impact on functionality
- etc.
Does it sound realistic ?
Re: SSL error trapping
Posted by st...@lycos.co.uk.
The command
apr_table_setn(r->notes, "ssl-access-forbidden", "1");
is incorrect, as we don't have access to the request (request_rec struct).
I suppose we could find it, but do we really need this line ?
----- Original Message -----
From: sternmarc@lycos.co.uk
To: dev@httpd.apache.org
Sent: Monday, May 09, 2005 4:03 PM
Subject: Re: SSL error trapping
Here is my final proposal.
I changed it a bit in order to be fully compatible with the current implementation.
Technical description (based on 2.0.54):
In ssl_io_filter_connect( ) - ssl_engine_io.c - we have 2 cases (at line 1147 and 1173) where the connection may break because of certificates verification/validation problem: ' return ssl_filter_io_shutdown(filter_ctx, c, 1); '
I would return only if the error trapping module (mod_ssl_error) is not loaded.
If it is loaded, I would accept the certificate (continue the treatment and return DECLINED), as the error will be trapped later.
It just may be needed to add "apr_table_setn(r->notes, "ssl-access-forbidden", "1");" ?
So, replace
return ssl_filter_io_shutdown(filter_ctx, c, 1);
by
if ( is_ssl_error_loaded ) apr_table_setn(r->notes, "ssl-access-forbidden", "1");
else return ssl_filter_io_shutdown(filter_ctx, c, 1);
In order to check if the module is loaded, I need a few lines at the beginning of the function - unless a function exists to check if a module is loaded ? Currently I coded it in the function:
BOOL sslErrorRedirected = FALSE;
{ /* Check if mod_ssl_error is loaded */
extern AP_DECLARE_DATA module *ap_top_module;
module *modp;
for ( modp = ap_top_module; modp; modp = modp->next )
if ( strcmp(modp->name, "mod_ssl_error.c") == 0 ) {
sslErrorRedirected = TRUE;
break;
}
}
Error trapping module:
Here is what I implemented:
1. Specific error page
<IfModule mod_ssl_error.c>
SSL_Error_URL 10 "/error/expired.html"
SSL_Error_URL 12 "/error/crl_expired.html"
SSL_Error_URL 23 "/error/revoked.html"
</IfModule>
This directive allow to redirect to a specific page, in case the error X is detected (X is the OpenSSL error code).
2. General error page
<IfModule mod_ssl_error.c>
SSL_Error_DefaultURL /error/ssl_valid.html
</IfModule>
This directive allow to redirect to a specific page, in case an error is detected and is not explicitely trapped with 1.
The error message is added to the URL: "/error/ssl_valid.html?error=XXX".
3. No error page
In case the "SSL_Error_DefaultURL" directive In case the "SSL_Error_DefaultURL" directive is not specified, it generates an error 403 (HTTP_FORBIDDEN).
URL
URL can be
- absolute HTTP => no change
- absolute HTTPS => change HTTPS to HTTP (to avoid loops)
- relative filename => add "http://hostname", where hostname comes from the request (request_rec struct)
Does anybody see possible enhancements ?
Currently it only traps certificate validation problems, but it may be extended to other non fatal SSL error if any (?)
Does it seem reasonable to include it as a patch in HEAD ?
Thanks for the feedback,
Marc
In case a SSL connection fails because a certificate is expired, or a CRL is unavailable, etc., the browser receives a SSL error that results in a cryptic technical error displayed to the user - sometimes only an error number like in Firefox. In such a situation, the SSL connection could be established, and a HTTP_FORBIDDEN (403) error returned. By adding another module, It is even possible to trap the exact SSL error and redirect to a page with the specific error message ("Your certificate is expired", "We cannot check the validity of the certificate - retry later", .).
Re: SSL error trapping
Posted by st...@lycos.co.uk.
Here is my final proposal.
I changed it a bit in order to be fully compatible with the current implementation.
Technical description (based on 2.0.54):
In ssl_io_filter_connect( ) - ssl_engine_io.c - we have 2 cases (at line 1147 and 1173) where the connection may break because of certificates verification/validation problem: ' return ssl_filter_io_shutdown(filter_ctx, c, 1); '
I would return only if the error trapping module (mod_ssl_error) is not loaded.
If it is loaded, I would accept the certificate (continue the treatment and return DECLINED), as the error will be trapped later.
It just may be needed to add "apr_table_setn(r->notes, "ssl-access-forbidden", "1");" ?
So, replace
return ssl_filter_io_shutdown(filter_ctx, c, 1);
by
if ( is_ssl_error_loaded ) apr_table_setn(r->notes, "ssl-access-forbidden", "1");
else return ssl_filter_io_shutdown(filter_ctx, c, 1);
In order to check if the module is loaded, I need a few lines at the beginning of the function - unless a function exists to check if a module is loaded ? Currently I coded it in the function:
BOOL sslErrorRedirected = FALSE;
{ /* Check if mod_ssl_error is loaded */
extern AP_DECLARE_DATA module *ap_top_module;
module *modp;
for ( modp = ap_top_module; modp; modp = modp->next )
if ( strcmp(modp->name, "mod_ssl_error.c") == 0 ) {
sslErrorRedirected = TRUE;
break;
}
}
Error trapping module:
Here is what I implemented:
1. Specific error page
<IfModule mod_ssl_error.c>
SSL_Error_URL 10 "/error/expired.html"
SSL_Error_URL 12 "/error/crl_expired.html"
SSL_Error_URL 23 "/error/revoked.html"
</IfModule>
This directive allow to redirect to a specific page, in case the error X is detected (X is the OpenSSL error code).
2. General error page
<IfModule mod_ssl_error.c>
SSL_Error_DefaultURL /error/ssl_valid.html
</IfModule>
This directive allow to redirect to a specific page, in case an error is detected and is not explicitely trapped with 1.
The error message is added to the URL: "/error/ssl_valid.html?error=XXX".
3. No error page
In case the "SSL_Error_DefaultURL" directive In case the "SSL_Error_DefaultURL" directive is not specified, it generates an error 403 (HTTP_FORBIDDEN).
URL
URL can be
- absolute HTTP => no change
- absolute HTTPS => change HTTPS to HTTP (to avoid loops)
- relative filename => add "http://hostname", where hostname comes from the request (request_rec struct)
Does anybody see possible enhancements ?
Currently it only traps certificate validation problems, but it may be extended to other non fatal SSL error if any (?)
Does it seem reasonable to include it as a patch in HEAD ?
Thanks for the feedback,
Marc
In case a SSL connection fails because a certificate is expired, or a CRL is unavailable, etc., the browser receives a SSL error that results in a cryptic technical error displayed to the user - sometimes only an error number like in Firefox. In such a situation, the SSL connection could be established, and a HTTP_FORBIDDEN (403) error returned. By adding another module, It is even possible to trap the exact SSL error and redirect to a page with the specific error message ("Your certificate is expired", "We cannot check the validity of the certificate - retry later", ..).