Merge branch 'master' of github.com:iNPUTmice/Conversations

Stephen Paul Weber created

* 'master' of github.com:iNPUTmice/Conversations:
  version bump to 2.10.5 + changelog
  retrieve uncompressed file size in HEAD request
  disable knownFileSize on re-download for pgp encrypted files
  limit posh files to 10k
  ensure downloaded file does not exceed Content-Length reported by HEAD

Change summary

CHANGELOG.md                                                              |  5 
src/main/java/eu/siacs/conversations/http/HttpDownloadConnection.java     | 37 
src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java | 19 
src/main/res/values/strings.xml                                           |  1 
4 files changed, 46 insertions(+), 16 deletions(-)

Detailed changes

CHANGELOG.md 🔗

@@ -1,5 +1,10 @@
 # Changelog
 
+### Version 2.10.5
+
+* Security: Stop downloading files that exceed advertised file size
+* Security: Limit POSH files to 10K
+
 ### Version 2.10.4
 
 * Fix interaction with Google Maps Share Location Plugin

src/main/java/eu/siacs/conversations/http/HttpDownloadConnection.java 🔗

@@ -102,11 +102,20 @@ public class HttpDownloadConnection implements Transferable {
             if (this.message.getEncryption() == Message.ENCRYPTION_AXOLOTL && this.file.getKey() == null) {
                 this.message.setEncryption(Message.ENCRYPTION_NONE);
             }
-            //TODO add auth tag size to knownFileSize
-            final Long knownFileSize = message.getFileParams().size;
+            final Long knownFileSize;
+            if (message.getEncryption() == Message.ENCRYPTION_PGP || message.getEncryption() == Message.ENCRYPTION_DECRYPTED) {
+                knownFileSize = null;
+            } else {
+                knownFileSize = message.getFileParams().size;
+            }
             Log.d(Config.LOGTAG,"knownFileSize: "+knownFileSize+", body="+message.getBody());
             if (knownFileSize != null && interactive) {
-                this.file.setExpectedSize(knownFileSize);
+                if (message.getEncryption() == Message.ENCRYPTION_AXOLOTL
+                        && this.file.getKey() != null) {
+                    this.file.setExpectedSize(knownFileSize + 16);
+                } else {
+                    this.file.setExpectedSize(knownFileSize);
+                }
                 download(true);
             } else {
                 checkFileSize(interactive);
@@ -216,6 +225,8 @@ public class HttpDownloadConnection implements Transferable {
             mXmppConnectionService.showErrorToastInUi(R.string.download_failed_could_not_connect);
         } else if (e instanceof FileWriterException) {
             mXmppConnectionService.showErrorToastInUi(R.string.download_failed_could_not_write_file);
+        } else if (e instanceof InvalidFileException) {
+            mXmppConnectionService.showErrorToastInUi(R.string.download_failed_invalid_file);
         } else {
             mXmppConnectionService.showErrorToastInUi(R.string.download_failed_file_not_found);
         }
@@ -311,6 +322,7 @@ public class HttpDownloadConnection implements Transferable {
             );
             final Request request = new Request.Builder()
                     .url(URL.stripFragment(mUrl))
+                    .addHeader("Accept-Encoding", "identity")
                     .head()
                     .build();
             mostRecentCall = client.newCall(request);
@@ -336,11 +348,11 @@ public class HttpDownloadConnection implements Transferable {
                     throw new IOException("Server reported negative file size");
                 }
                 return size;
-            } catch (IOException e) {
+            } catch (final IOException e) {
                 Log.d(Config.LOGTAG, "io exception during HEAD " + e.getMessage());
                 throw e;
-            } catch (NumberFormatException e) {
-                throw new IOException();
+            } catch (final NumberFormatException e) {
+                throw new IOException(e);
             }
         }
 
@@ -428,9 +440,12 @@ public class HttpDownloadConnection implements Transferable {
                 transmitted += count;
                 try {
                     outputStream.write(buffer, 0, count);
-                } catch (IOException e) {
+                } catch (final IOException e) {
                     throw new FileWriterException(file);
                 }
+                if (transmitted > expected) {
+                    throw new InvalidFileException(String.format("File exceeds expected size of %d", expected));
+                }
                 updateProgress(Math.round(((double) transmitted / expected) * 100));
             }
             outputStream.flush();
@@ -458,4 +473,12 @@ public class HttpDownloadConnection implements Transferable {
             throw new IOException(String.format(Locale.ENGLISH, "HTTP Status code was %d", code));
         }
     }
+
+    private static class InvalidFileException extends IOException {
+
+        private InvalidFileException(final String message) {
+            super(message);
+        }
+
+    }
 }

src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java 🔗

@@ -43,6 +43,7 @@ import androidx.appcompat.app.AppCompatActivity;
 
 import com.google.common.base.Charsets;
 import com.google.common.base.Joiner;
+import com.google.common.io.ByteStreams;
 import com.google.common.io.CharStreams;
 
 import org.json.JSONArray;
@@ -77,6 +78,7 @@ import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
 import javax.net.ssl.X509TrustManager;
 
+import eu.siacs.conversations.Config;
 import eu.siacs.conversations.R;
 import eu.siacs.conversations.crypto.XmppDomainVerifier;
 import eu.siacs.conversations.entities.MTMDecision;
@@ -391,13 +393,13 @@ public class MemorizingTrustManager {
                     final List<String> fingerprints = getPoshFingerprints(domain);
                     if (hash != null && fingerprints.size() > 0) {
                         if (fingerprints.contains(hash)) {
-                            Log.d("mtm", "trusted cert fingerprint of " + domain + " via posh");
+                            Log.d(Config.LOGTAG, "trusted cert fingerprint of " + domain + " via posh");
                             return;
                         } else {
-                            Log.d("mtm", "fingerprint " + hash + " not found in " + fingerprints);
+                            Log.d(Config.LOGTAG, "fingerprint " + hash + " not found in " + fingerprints);
                         }
                         if (getPoshCacheFile(domain).delete()) {
-                            Log.d("mtm", "deleted posh file for " + domain + " after not being able to verify");
+                            Log.d(Config.LOGTAG, "deleted posh file for " + domain + " after not being able to verify");
                         }
                     }
                 }
@@ -410,7 +412,7 @@ public class MemorizingTrustManager {
         }
     }
 
-    private List<String> getPoshFingerprints(String domain) {
+    private List<String> getPoshFingerprints(final String domain) {
         final List<String> cached = getPoshFingerprintsFromCache(domain);
         if (cached == null) {
             return getPoshFingerprintsFromServer(domain);
@@ -424,13 +426,13 @@ public class MemorizingTrustManager {
     }
 
     private List<String> getPoshFingerprintsFromServer(String domain, String url, int maxTtl, boolean followUrl) {
-        Log.d("mtm", "downloading json for " + domain + " from " + url);
+        Log.d(Config.LOGTAG, "downloading json for " + domain + " from " + url);
         final SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(master);
         final boolean useTor = QuickConversationsService.isConversations() && preferences.getBoolean("use_tor", master.getResources().getBoolean(R.bool.use_tor));
         try {
             final List<String> results = new ArrayList<>();
             final InputStream inputStream = HttpConnectionManager.open(url, useTor);
-            final String body = CharStreams.toString(new InputStreamReader(inputStream, Charsets.UTF_8));
+            final String body = CharStreams.toString(new InputStreamReader(ByteStreams.limit(inputStream,10_000), Charsets.UTF_8));
             final JSONObject jsonObject = new JSONObject(body);
             int expires = jsonObject.getInt("expires");
             if (expires <= 0) {
@@ -457,7 +459,7 @@ public class MemorizingTrustManager {
             writeFingerprintsToCache(domain, results, 1000L * expires + System.currentTimeMillis());
             return results;
         } catch (final Exception e) {
-            Log.d("mtm", "error fetching posh " + e.getMessage());
+            Log.d(Config.LOGTAG, "error fetching posh",e);
             return new ArrayList<>();
         }
     }
@@ -495,7 +497,7 @@ public class MemorizingTrustManager {
                 file.delete();
                 return null;
             } else {
-                Log.d("mtm", "posh fingerprints expire in " + (expiresIn / 1000) + "s");
+                Log.d(Config.LOGTAG, "posh fingerprints expire in " + (expiresIn / 1000) + "s");
             }
             final List<String> result = new ArrayList<>();
             final JSONArray jsonArray = jsonObject.getJSONArray("fingerprints");
@@ -512,7 +514,6 @@ public class MemorizingTrustManager {
     }
 
     private X509Certificate[] getAcceptedIssuers() {
-        LOGGER.log(Level.FINE, "getAcceptedIssuers()");
         return defaultTrustManager == null ? new X509Certificate[0] : defaultTrustManager.getAcceptedIssuers();
     }
 

src/main/res/values/strings.xml 🔗

@@ -464,6 +464,7 @@
     <string name="download_failed_file_not_found">Download failed: File not found</string>
     <string name="download_failed_could_not_connect">Download failed: Could not connect to host</string>
     <string name="download_failed_could_not_write_file">Download failed: Could not write file</string>
+    <string name="download_failed_invalid_file">Download failed: Invalid file</string>
     <string name="account_status_tor_unavailable">Tor network unavailable</string>
     <string name="account_status_bind_failure">Bind failure</string>
     <string name="account_status_host_unknown">The server is not responsible for this domain</string>