Bug 577725 - Possible corruption of Secure Storage file after Change Password

After using the "Change Password..." button of the Secure Storage
preference page the secure_storage file could end-up corrupted if
the application quits unexpectedly (kill -9, Ctrl+C, Force Quit
in macOS or a crash).

After being corrupted, the following kind of exception is thrown:

org.eclipse.equinox.security.storage.StorageException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.
Caused by: javax.crypto.BadPaddingException: Given final block not properly padded. Such issues can arise if a bad key is used during decryption.

This was caused due to not flushing the re-encrypted entries, just the
verification node updated by the master password provider.

Flushing the entries at the end of the re-encryption prevents such
scenarios.

Changed the service segment of the versions to fix Jenkins build
complaining that 'Only qualifier changed for ...'

Signed-off-by: Martin D'Aloia <martindaloia@gmail.com>
Change-Id: Icca6d98e05fca02f35e9f93242d57623d79622c4
Reviewed-on: https://git.eclipse.org/r/c/equinox/rt.equinox.bundles/+/188996
Tested-by: Thomas Watson <tjwatson@us.ibm.com>
Reviewed-by: Thomas Watson <tjwatson@us.ibm.com>
diff --git a/bundles/org.eclipse.equinox.security.tests/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.security.tests/META-INF/MANIFEST.MF
index 19b4886..2d82821 100644
--- a/bundles/org.eclipse.equinox.security.tests/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.equinox.security.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@
 Bundle-ManifestVersion: 2
 Bundle-Name: Equinox security tests
 Bundle-SymbolicName: org.eclipse.equinox.security.tests;singleton:=true
-Bundle-Version: 1.2.200.qualifier
+Bundle-Version: 1.2.300.qualifier
 Bundle-Activator: org.eclipse.equinox.internal.security.tests.SecurityTestsActivator
 Bundle-RequiredExecutionEnvironment: JavaSE-11
 Bundle-Vendor: Eclipse.org
diff --git a/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/META-INF/MANIFEST.MF
new file mode 100644
index 0000000..ecf1a90
--- /dev/null
+++ b/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/META-INF/MANIFEST.MF
@@ -0,0 +1,7 @@
+Manifest-Version: 1.0
+Bundle-ManifestVersion: 2
+Bundle-Name: ControlledPasswordProvider
+Bundle-SymbolicName: org.eclipse.equinox.security.ControlledPasswordProvider;singleton:=true
+Bundle-Version: 1.0.0.qualifier
+Bundle-Vendor: Eclipse.org
+Require-Bundle: org.eclipse.equinox.security.tests
diff --git a/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/plugin.xml b/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/plugin.xml
new file mode 100644
index 0000000..4197da8
--- /dev/null
+++ b/bundles/org.eclipse.equinox.security.tests/Plugin_Testing/controlled_provider/plugin.xml
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<?eclipse version="3.2"?>
+<plugin>
+   <extension
+         id="ControlledPasswordProvider"
+         name="Controlled password provider for tests of secure preferences"
+         point="org.eclipse.equinox.security.secureStorage">
+      <provider
+            class="org.eclipse.equinox.internal.security.tests.storage.ControlledPasswordProvider"
+            priority="10">
+      </provider>
+   </extension>
+   
+</plugin>
diff --git a/bundles/org.eclipse.equinox.security.tests/pom.xml b/bundles/org.eclipse.equinox.security.tests/pom.xml
index b66d71a..d692f1f 100644
--- a/bundles/org.eclipse.equinox.security.tests/pom.xml
+++ b/bundles/org.eclipse.equinox.security.tests/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.equinox</groupId>
   <artifactId>org.eclipse.equinox.security.tests</artifactId>
-  <version>1.2.200-SNAPSHOT</version>
+  <version>1.2.300-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
   <properties>
   	<testClass>org.eclipse.equinox.security.tests.AllSecurityTests</testClass>
diff --git a/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ControlledPasswordProvider.java b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ControlledPasswordProvider.java
new file mode 100644
index 0000000..442af33
--- /dev/null
+++ b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ControlledPasswordProvider.java
@@ -0,0 +1,61 @@
+/*******************************************************************************
+ * Copyright (c) 2008, 2018 IBM Corporation and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ * 
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.equinox.internal.security.tests.storage;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import javax.crypto.spec.PBEKeySpec;
+import org.eclipse.equinox.security.storage.provider.IPreferencesContainer;
+import org.eclipse.equinox.security.storage.provider.PasswordProvider;
+
+/**
+ * Password provider which can be provide controlled password generation from
+ * tests.
+ * <p>
+ * Initially the password is 'initialPassword' and on each request to generate a
+ * new password it will be 'changedPassword-X' where X is a incremental number.
+ */
+public class ControlledPasswordProvider extends PasswordProvider {
+
+	// MODULE_ID = lower-case(Bundle-SymbolicName + Extension ID)
+	public static final String MODULE_ID = "org.eclipse.equinox.security.controlledpasswordprovider.controlledpasswordprovider";
+
+	private static final String INITIAL_PASSWORD = "initialPassword";
+	private static final String CHANGED_PASSWORD = "changedPassword-"; // An incremental number will be appended
+
+	private static PBEKeySpec PASSWORD = new PBEKeySpec(INITIAL_PASSWORD.toCharArray());
+	private static AtomicInteger counter = new AtomicInteger(0);
+
+	@Override
+	public PBEKeySpec getPassword(IPreferencesContainer container, int passwordType) {
+
+		boolean newPassword = ((passwordType & CREATE_NEW_PASSWORD) != 0);
+		boolean passwordChange = ((passwordType & PASSWORD_CHANGE) != 0);
+
+		if (newPassword || passwordChange) {
+			PASSWORD = new PBEKeySpec(createNewPassword().toCharArray());
+		}
+
+		return PASSWORD;
+	}
+
+	@Override
+	public boolean retryOnError(Exception e, IPreferencesContainer container) {
+		return false;
+	}
+
+	private String createNewPassword() {
+		return CHANGED_PASSWORD + counter.incrementAndGet();
+	}
+
+}
diff --git a/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ReEncrypterTest.java b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ReEncrypterTest.java
new file mode 100644
index 0000000..83f4ce2
--- /dev/null
+++ b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/internal/security/tests/storage/ReEncrypterTest.java
@@ -0,0 +1,152 @@
+/*******************************************************************************
+ * Copyright (c) 2008 IBM Corporation and others.
+ *
+ * This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License 2.0
+ * which accompanies this distribution, and is available at
+ * https://www.eclipse.org/legal/epl-2.0/
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ * 
+ * Contributors:
+ *     IBM Corporation - initial API and implementation
+ *******************************************************************************/
+package org.eclipse.equinox.internal.security.tests.storage;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.util.Map;
+import org.eclipse.core.tests.harness.BundleTestingHelper;
+import org.eclipse.equinox.internal.security.storage.friends.InternalExchangeUtils;
+import org.eclipse.equinox.internal.security.storage.friends.ReEncrypter;
+import org.eclipse.equinox.internal.security.tests.SecurityTestsActivator;
+import org.eclipse.equinox.security.storage.ISecurePreferences;
+import org.eclipse.equinox.security.storage.StorageException;
+import org.junit.Test;
+import org.osgi.framework.*;
+
+public class ReEncrypterTest extends StorageAbstractTest {
+
+	final private static int MAX_TIME_PER_BUNDLE = 10000; // maximum time to wait for bundle event in milliseconds
+	final private static String TEST_FILES_ROOT = "Plugin_Testing/";
+
+	final private static String key = "password";
+	final private static String value = "p[[pkknb#";
+
+	final private static String clearTextKey = "data";
+	final private static String clearTextValue = "-> this should not be encrypted <-";
+
+	final private static String defaultValue = "default";
+
+	@Test
+	public void testFlushAfterEncrypt() throws IOException, StorageException, BundleException {
+		URL location = getStorageLocation();
+		assertNotNull(location);
+
+		Bundle bundle = null;
+		try {
+			bundle = installBundle("controlled_provider");
+
+			{ // block1: fill and save
+				ISecurePreferences preferences = newPreferences(location, getOptions());
+				fill(preferences);
+				preferences.flush();
+				closePreferences(preferences);
+			}
+
+			{ // block2: re-encrypt
+				ISecurePreferences preferences = newPreferences(location, getOptions());
+				ReEncrypter reEncrypter = new ReEncrypter(preferences, getModuleID());
+
+				boolean decryptResult = reEncrypter.decrypt();
+				assertTrue(decryptResult);
+
+				boolean switchToNewPasswordResult = reEncrypter.switchToNewPassword();
+				assertTrue(switchToNewPasswordResult);
+
+				boolean encryptResult = reEncrypter.encrypt();
+				assertTrue(encryptResult);
+			}
+
+			{ // block3: re-load and check
+				ISecurePreferences preferences = newPreferences(location, getOptions());
+				check(preferences);
+			}
+		} finally {
+			if (bundle != null)
+				bundle.uninstall();
+		}
+	}
+
+	@Override
+	protected String getModuleID() {
+		return ControlledPasswordProvider.MODULE_ID;
+	}
+
+	protected Map<String, Object> getOptions() {
+		// Don't specify default password for those tests; they need to have
+		// password providers
+		return getOptions(null);
+	}
+
+	/**
+	 * Fills the secure preferences with some encrypted and non encrypted values.
+	 */
+	private void fill(ISecurePreferences preferences) throws StorageException {
+		assertFalse(isModified(preferences));
+
+		preferences.put(key, value, true); // puts encrypted entry at the root node
+		preferences.put(clearTextKey, clearTextValue, false); // puts clear text entry at the root node
+
+		assertTrue(isModified(preferences));
+	}
+
+	/**
+	 * Checks that there isn't any change in the secure preferences and it contains
+	 * the same values preciously saved.
+	 */
+	private void check(ISecurePreferences preferences) throws StorageException {
+		assertFalse(isModified(preferences));
+
+		assertEquals(value, preferences.get(key, defaultValue)); // checks entry at the root node
+		assertEquals(clearTextValue, preferences.get(clearTextKey, defaultValue));
+	}
+
+	/**
+	 * The method reaches into internal classes to check if modified flag is set on
+	 * secure preference data.
+	 */
+	private boolean isModified(ISecurePreferences node) {
+		return InternalExchangeUtils.isModified(node);
+	}
+
+	/**
+	 * Dynamically installs a bundle that should contribute an Extension to the
+	 * org.eclipse.equinox.security.secureStorage Extension Point.
+	 * 
+	 * Copied from DynamicPreferencesTest.
+	 */
+	protected Bundle installBundle(String bundlePath) throws MalformedURLException, BundleException, IOException {
+		BundleContext bundleContext = SecurityTestsActivator.getDefault().getBundleContext();
+		Bundle bundle = null;
+		WaitingRegistryListener listener = new WaitingRegistryListener();
+		listener.register("org.eclipse.equinox.security.secureStorage");
+
+		try {
+			bundle = BundleTestingHelper.installBundle("0.1", bundleContext, TEST_FILES_ROOT + bundlePath);
+			BundleTestingHelper.refreshPackages(bundleContext, new Bundle[] { bundle });
+			// synchronization: listener should receive 1 group of events
+			assertTrue(listener.waitFor(1, MAX_TIME_PER_BUNDLE) == 1);
+		} finally {
+			listener.unregister();
+		}
+		return bundle;
+	}
+
+}
diff --git a/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/security/tests/AllSecurityTests.java b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/security/tests/AllSecurityTests.java
index 7e84a7f..d51cbdd 100644
--- a/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/security/tests/AllSecurityTests.java
+++ b/bundles/org.eclipse.equinox.security.tests/src/org/eclipse/equinox/security/tests/AllSecurityTests.java
@@ -26,7 +26,7 @@
  */
 @RunWith(Suite.class)
 @SuiteClasses({ Base64Test.class, DetectPBECiphersTest.class, SlashEncodeTest.class, DefaultPreferencesTest.class,
-		DynamicPreferencesTest.class, ObsoletesTest.class, WinPreferencesTest.class })
+		DynamicPreferencesTest.class, ObsoletesTest.class, WinPreferencesTest.class, ReEncrypterTest.class })
 public class AllSecurityTests {
 	// see @SuiteClasses
 }
diff --git a/bundles/org.eclipse.equinox.security/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.security/META-INF/MANIFEST.MF
index 8eaaade..df46f58 100644
--- a/bundles/org.eclipse.equinox.security/META-INF/MANIFEST.MF
+++ b/bundles/org.eclipse.equinox.security/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.equinox.security;singleton:=true
-Bundle-Version: 1.3.800.qualifier
+Bundle-Version: 1.3.900.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Bundle-Activator: org.eclipse.equinox.internal.security.auth.AuthPlugin
diff --git a/bundles/org.eclipse.equinox.security/pom.xml b/bundles/org.eclipse.equinox.security/pom.xml
index 578bbfe..63c2699 100644
--- a/bundles/org.eclipse.equinox.security/pom.xml
+++ b/bundles/org.eclipse.equinox.security/pom.xml
@@ -19,7 +19,7 @@
   </parent>
   <groupId>org.eclipse.equinox</groupId>
   <artifactId>org.eclipse.equinox.security</artifactId>
-  <version>1.3.800-SNAPSHOT</version>
+  <version>1.3.900-SNAPSHOT</version>
   <packaging>eclipse-plugin</packaging>
 
   <build>
diff --git a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/SecAuthMessages.java b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/SecAuthMessages.java
index 657ae4b..e071fd9 100644
--- a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/SecAuthMessages.java
+++ b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/SecAuthMessages.java
@@ -87,6 +87,7 @@
 	public static String usingAlgorithm;
 	public static String decryptingError;
 	public static String encryptingError;
+	public static String persistingError;
 	public static String noDigest;
 	public static String failedCreateRecovery;
 	public static String initCancelled;
diff --git a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/messages.properties b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/messages.properties
index 2188284..b52345b 100644
--- a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/messages.properties
+++ b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/auth/nls/messages.properties
@@ -80,6 +80,7 @@
 usingAlgorithm = Unable to find default encryption algorithm \"{0}\", using \"{1}\" instead.
 decryptingError = Unable to decrypt value associated with the key \"{0}\" on the node \"{1}\".
 encryptingError = Unable to encrypt value associated with the key \"{0}\" on the node \"{1}\".
+persistingError = Unable to persist encrypted values of node \"{0}\" to the underlying storage.
 noDigest = Digest algorithm  \"{0}\" is not available.
 failedCreateRecovery = Unable to create value for the password recovery.
 initCancelled = Secure Storage initialization was canceled; please try again.
diff --git a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/storage/friends/ReEncrypter.java b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/storage/friends/ReEncrypter.java
index 6fe0bc8..d504437 100644
--- a/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/storage/friends/ReEncrypter.java
+++ b/bundles/org.eclipse.equinox.security/src/org/eclipse/equinox/internal/security/storage/friends/ReEncrypter.java
@@ -13,6 +13,7 @@
  *******************************************************************************/
 package org.eclipse.equinox.internal.security.storage.friends;
 
+import java.io.IOException;
 import java.util.*;
 import java.util.Map.Entry;
 import org.eclipse.equinox.internal.security.auth.AuthPlugin;
@@ -143,6 +144,16 @@
 			container.setOption(IProviderHints.REQUIRED_MODULE_ID, originalProperty);
 		else
 			container.removeOption(IProviderHints.REQUIRED_MODULE_ID);
+
+		try {
+			// Ensure modified entries are persisted and avoid potential inconsistent state
+			root.flush();
+		} catch (IOException e) {
+			String msg = NLS.bind(SecAuthMessages.persistingError, root.name());
+			AuthPlugin.getDefault().logError(msg, e);
+			result = false;
+		}
+
 		return result;
 	}