[Privoxy-devel] HTTPS filtering in Privoxy

[Fabian Keil]

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:

- The commit message "final change" is meaningless and should
  be changed to reflect what the commit does.
- 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.).
- 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.
- 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.
- The added action(s) need documentation (in doc/source/user-manual.sgml).
- 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.

- 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.
- 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.
- 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.

- 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?
- The code in ssl_debug_callback() is completely commented
  out. Probably the callback shouldn't be set instead
  (or set based on an option).
- 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.
- 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.
- Many functions in ssl.c only need a small part of csp
  in which case they shouldn't get access to the whole
  struct.
- The code has to be rebased on CVS HEAD before it can
  be committed.

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

Is the bachelor thesis public?

Thanks,
Fabian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.privoxy.org/pipermail/privoxy-devel/attachments/20170518/045e289d/attachment.bin>