[Privoxy-devel] HTTPS filtering in Privoxy

[Vašek Švec]

On Thu, May 18, 2017 at 2:22 PM, Fabian Keil <fk at fabiankeil.de> wrote:

> Vašek Švec <va.svec at gmail.com> wrote:
>
> > file with patches (git format-patch) is in attachment. I hereby release
> > this code under the GNU GPLv2 or later.
>
> Thanks a lot for the patch.
>
> Unfortunately Mailman stripped it due to the mime type
> (application/octet-stream) but I received it as part of the
> moderation message an will attach it (renamed) to this mail
> so other list subscribers can take a look at it as well.
>
> A couple of comments after looking at the patch without
> testing it yet:
>
> I agree with most of yours comments. However I'm just learning for my
final exams and I don't have enough time to work on this codes or
documentation. From the end of June, I should have more free time and then
I'd like to continue this work. It would be quicker if these changes were
made by someone else.


> - The commit message "final change" is meaningless and should
>   be changed to reflect what the commit does.



I wasn't using VCS for changes. This commit was created yesterday for
purpose of this diff file.


> - The various "Changed by ..." markers inside the code are not
>   appropriate as they don't help to understand the code.
>   Credit should be given through other means (copyright header
>   where relevant, VCS system, commit message, change log etc.).
>

This markers was created only for purpose of final thesis. They should be
deleted.


> - The TLS/SSL code should be optional at compile time and
>   run time so it doesn't affect users who don't need it
>   or can't satisfy the added library dependency.
>   It should be disabled by default to reduce the chances that
>   it's used without understanding the implications.
>

 I agree. Unfortunately I don't have enough time to fix it right now.

- Some of the modified files are generated (ChangeLog, INSTALL, config).
>   The changes have to be added to the source files in doc/source instead.

- The patch adds two separate boolean actions (disable-ssl-filtering
>   and ignore-certificate-errors) that probably shouldn't be separated.
>
>   A multi action like filter-encrypted-content with a parameter that
>   specifies whether or not certificates should be validates seems more
>   appropriate to me.
>

Maybe it would be better solution. I didn't realize that these parameters
could/should be merged.


> - The added action(s) need documentation (in doc/source/user-manual.sgml).
>

Yes, it definitely should be, but I didn't change anything in documentation
for now. I'm not really sure, if my English is good enough to be published
in documentation.


> - There are several chunks for jcc.c like:
>
> +               /*
> +                * Changed by Vaclav Svec
> +                * Sending data with standard or secured connection
> (HTTP/HTTPS)
> +                */
> +               if (client_use_ssl(csp))
> +               {
> +                  ssl_send_data(&(csp->mbedtls_client_attr.ssl),
> +                     (const unsigned char *)INVALID_SERVER_HEADERS_
> RESPONSE,
> +                     strlen(INVALID_SERVER_HEADERS_RESPONSE));
> +               }
> +               else
> +               {
> +                  write_socket(csp->cfd, INVALID_SERVER_HEADERS_RESPONSE,
> +                     strlen(INVALID_SERVER_HEADERS_RESPONSE));
> +               }
>
> Adding a separate function that decides which write function to
> use would reduce the code duplication.
>
> Agree. Should be in one function.


> - This chunk for jcc.c seems dubious:
>
> +            if (!csp->headers->first->str) {
> +               log_error(LOG_LEVEL_ERROR, "header search:
> csp->headers->first->str == NULL, assert will be called");
> +            }
>              assert(csp->headers->first->str);
>   The added message is wrong unless assertions are enabled.
>
>   Without your code the assertion isn't expected to be triggered
>   at all, if your code changes this assumption this has to be
>   handled without crashing.
>

Probably my forgotten log.


> - In a couple of chunks the indentation of related lines is changed
>   like in:
> @@ -2706,41 +3155,62 @@ static void chat(struct client_state *csp)
>                  * delivered the crunch response to the client
>                  * and are done here after cleaning up.
>                  */
> -                freez(hdr);
> -                mark_server_socket_tainted(csp);
> -                return;
> +               freez(hdr);
> +               mark_server_socket_tainted(csp);
> +               close_client_and_server_ssl_connections(csp);
> +               return;
>              }
> The indentation fixes belong in separate commits.
>

I wasn't using VCS for during implementation.


> - Many chunks add trailing white space which should be removed.
> - Chunks like this look dubious to me:
>
> @@ -3256,6 +3743,20 @@ static void initialize_mutexes(void)
>     /*
>      * Prepare global mutex semaphores
>      */
> +
> +   /*Added by Vaclav Svec*/
> +#ifdef LIMIT_MUTEX_NUMBER
> +   int i = 0;
> +   for (i = 0; i < 32; i++)
> +#else
> +   int i = 0;
> +   for (i = 0; i < 65536; i++)
> +#endif /* LIMIT_MUTEX_NUMBER */
>
> It's not obvious to me why 32 and 65536 are used here.
>
> It seems preferable to me to use a single define for the
> number of certificates_mutexes[] slots and use sizeof or
> the define for the loop boundaries.


I used these two values to simplify the calculation of used mutex number.
(in function get_certificate_mutex_id) The number is calculated from bytes
of host hash.  65536 - first two bytes of hash, and 32 is fallback for
platform which must save mutexes count.


> - In several places strdup() is used without checking
>   for failures. strdup_or_die() should be used instead.

- CVS ids shouldn't be modified manually. It can cause
>   merge conflicts and the changes will be automatically
>   overwritten anyway.
> - The copyright header in ssl.c and ssl.h is unclear.
>   Is "FIT CVUT" supposed to be a copyright holder?
>

I'm implementation author and FIT CVUT is there only mentioned to emphasize
that I'm a student of this faculty (and university). FIT CVUT isn't a
copyright holder.


> - The code in ssl_debug_callback() is completely commented
>   out. Probably the callback shouldn't be set instead
>   (or set based on an option).


Yes, it's possible solution.

>

- The format of many comments above functions in ssl.c
>   doesn't fit the usual template.
> - Variables should be declared at the beginning of the
>   block in which they are used.
> - seed_rng() should probably be called once from main()
>   in which case it wouldn't need the mutex.
>

Yes, it would be better solution.


> - In many places "if(" is used instead of "if (".
> - create_server_ssl_connection() seems to parse the CA
>   file for each connection which seems wasteful. Probably
>   the file content should be cached and only reloaded when
>   the file changed.
>

I agree, but I'm not able to implement this feature right now. I'm also not
sure how could be this functionality implemented.


> - Many functions in ssl.c only need a small part of csp
>   in which case they shouldn't get access to the whole
>   struct.
>

Yes. Maybe there could be created new structure (maybe as part of csp) with
necessary parameters for SSL connections.


> - The code has to be rebased on CVS HEAD before it can
>   be committed.
>

I don't have many experiences with CVS, therefore I didn't use it.

>
> Do you have time and interest to address these change
> requests or a subset of them and do they seem reasonable
> to you?
>

These days, I don't have enough time to work on these changes
(approximately next one and half month). In future, I'd like to work on
these changes or new features for SSL.

>
> Is the bachelor thesis public?
>

Yes, it's public, but it's written in Czech. I send it as an attachment
(because I don't know URL for public).

Václav Švec