From ea29f580a5e7cc854d1d5fe8b60554360b1a7625 Mon Sep 17 00:00:00 2001 From: Kyle Spearrin Date: Tue, 9 Nov 2021 15:37:58 -0500 Subject: [PATCH 1/7] clean api url paths from directory traversal (#539) --- common/src/services/api.service.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/src/services/api.service.ts b/common/src/services/api.service.ts index b7fff0ce..df33c179 100644 --- a/common/src/services/api.service.ts +++ b/common/src/services/api.service.ts @@ -1616,6 +1616,9 @@ export class ApiService implements ApiServiceAbstraction { headers.set('User-Agent', this.customUserAgent); } + // Clean path from directory traversal + path = path.split('../').join(''); + const requestInit: RequestInit = { cache: 'no-store', credentials: this.getCredentials(), From 1b4a5508bdd998a0acd7c45c1ddf9956e3f350d8 Mon Sep 17 00:00:00 2001 From: Kyle Spearrin Date: Wed, 10 Nov 2021 13:37:31 -0500 Subject: [PATCH 2/7] Revert "clean api url paths from directory traversal (#539)" This reverts commit ea29f580a5e7cc854d1d5fe8b60554360b1a7625. --- common/src/services/api.service.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/common/src/services/api.service.ts b/common/src/services/api.service.ts index df33c179..b7fff0ce 100644 --- a/common/src/services/api.service.ts +++ b/common/src/services/api.service.ts @@ -1616,9 +1616,6 @@ export class ApiService implements ApiServiceAbstraction { headers.set('User-Agent', this.customUserAgent); } - // Clean path from directory traversal - path = path.split('../').join(''); - const requestInit: RequestInit = { cache: 'no-store', credentials: this.getCredentials(), From b99103d3f7a050cd31e198e3860d432ba01a327d Mon Sep 17 00:00:00 2001 From: Kyle Spearrin Date: Wed, 10 Nov 2021 15:13:13 -0500 Subject: [PATCH 3/7] validate path for directory traversal (#540) * validate path for directory traversal * use previously constructed requestUrl --- common/src/services/api.service.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/common/src/services/api.service.ts b/common/src/services/api.service.ts index b7fff0ce..a2c8ee4a 100644 --- a/common/src/services/api.service.ts +++ b/common/src/services/api.service.ts @@ -1609,6 +1609,13 @@ export class ApiService implements ApiServiceAbstraction { authed: boolean, hasResponse: boolean, apiUrl?: string, alterHeaders?: (headers: Headers) => void): Promise { apiUrl = Utils.isNullOrWhitespace(apiUrl) ? this.environmentService.getApiUrl() : apiUrl; + + const requestUrl = apiUrl + path; + // Prevent directory traversal from malicious paths + if (new URL(requestUrl).href !== requestUrl) { + return Promise.reject('Invalid request url path.'); + } + const headers = new Headers({ 'Device-Type': this.deviceType, }); @@ -1647,7 +1654,7 @@ export class ApiService implements ApiServiceAbstraction { } requestInit.headers = headers; - const response = await this.fetch(new Request(apiUrl + path, requestInit)); + const response = await this.fetch(new Request(requestUrl, requestInit)); if (hasResponse && response.status === 200) { const responseJson = await response.json(); From e02e663ce1aed94d42db00dcdb2e42bdd625f0dc Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Fri, 12 Nov 2021 05:59:01 +1000 Subject: [PATCH 4/7] [Linked Fields] Fix QA feedback (#542) * Fix bug overwriting custom field types * Add linkedId to export model for CLI --- angular/src/components/add-edit-custom-fields.component.ts | 2 +- common/src/models/export/field.ts | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/angular/src/components/add-edit-custom-fields.component.ts b/angular/src/components/add-edit-custom-fields.component.ts index e1e4d149..71040ac4 100644 --- a/angular/src/components/add-edit-custom-fields.component.ts +++ b/angular/src/components/add-edit-custom-fields.component.ts @@ -118,7 +118,7 @@ export class AddEditCustomFieldsComponent implements OnChanges { } this.cipher.fields - .filter(f => f.type = FieldType.Linked) + .filter(f => f.type === FieldType.Linked) .forEach(f => f.linkedId = this.linkedFieldOptions[0].value); } } diff --git a/common/src/models/export/field.ts b/common/src/models/export/field.ts index 2e98c1bd..0804c123 100644 --- a/common/src/models/export/field.ts +++ b/common/src/models/export/field.ts @@ -1,4 +1,5 @@ import { FieldType } from '../../enums/fieldType'; +import { LinkedIdType } from '../../enums/linkedIdType'; import { FieldView } from '../view/fieldView'; @@ -18,6 +19,7 @@ export class Field { view.type = req.type; view.value = req.value; view.name = req.name; + view.linkedId = req.linkedId; return view; } @@ -25,12 +27,14 @@ export class Field { domain.type = req.type; domain.value = req.value != null ? new EncString(req.value) : null; domain.name = req.name != null ? new EncString(req.name) : null; + domain.linkedId = req.linkedId; return domain; } name: string; value: string; type: FieldType; + linkedId: LinkedIdType; constructor(o?: FieldView | FieldDomain) { if (o == null) { @@ -45,5 +49,6 @@ export class Field { this.value = o.value?.encryptedString; } this.type = o.type; + this.linkedId = o.linkedId; } } From 06c9df97adae203804c5d2cba23c0b0cc20a4b0a Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 15 Nov 2021 19:49:19 +1000 Subject: [PATCH 5/7] Update Safari importer to be Safari and macOS importer (#550) * Rename Safari importer to Safari and macOS * Order featured import options alphabetically --- common/src/services/import.service.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/common/src/services/import.service.ts b/common/src/services/import.service.ts index 3e797031..d04ede50 100644 --- a/common/src/services/import.service.ts +++ b/common/src/services/import.service.ts @@ -85,13 +85,13 @@ export class ImportService implements ImportServiceAbstraction { featuredImportOptions = [ { id: 'bitwardenjson', name: 'Bitwarden (json)' }, { id: 'bitwardencsv', name: 'Bitwarden (csv)' }, - { id: 'lastpasscsv', name: 'LastPass (csv)' }, { id: 'chromecsv', name: 'Chrome (csv)' }, - { id: 'firefoxcsv', name: 'Firefox (csv)' }, - { id: 'safaricsv', name: 'Safari (csv)' }, - { id: 'keepass2xml', name: 'KeePass 2 (xml)' }, - { id: '1password1pif', name: '1Password (1pif)' }, { id: 'dashlanejson', name: 'Dashlane (json)' }, + { id: 'firefoxcsv', name: 'Firefox (csv)' }, + { id: 'keepass2xml', name: 'KeePass 2 (xml)' }, + { id: 'lastpasscsv', name: 'LastPass (csv)' }, + { id: 'safaricsv', name: 'Safari and macOS (csv)' }, + { id: '1password1pif', name: '1Password (1pif)' }, ]; regularImportOptions: ImportOption[] = [ From 386903f5a9ef42fed709813d40e550ba91c0ee40 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Tue, 16 Nov 2021 07:53:57 +1000 Subject: [PATCH 6/7] [Key Connector] QA fixes for CLI and Desktop (#544) * Make UserVerificationService compatible with CLI * Refactor error handling * Fix i18n key name * Add apiUseKeyConnector flag to TokenResponse * Always require keyConnectorUrl to be passed in * Throw errors in userVerificationService * Use requestOTP in UserVerificationService * Remove unused deps * Fix linting --- .../verify-master-password.component.ts | 6 ++- .../abstractions/userVerification.service.ts | 1 + .../models/response/identityTokenResponse.ts | 2 + common/src/services/auth.service.ts | 5 ++- common/src/services/keyConnector.service.ts | 14 +------ .../src/services/userVerification.service.ts | 39 +++++++++---------- 6 files changed, 31 insertions(+), 36 deletions(-) diff --git a/angular/src/components/verify-master-password.component.ts b/angular/src/components/verify-master-password.component.ts index 01c511c9..658f7f59 100644 --- a/angular/src/components/verify-master-password.component.ts +++ b/angular/src/components/verify-master-password.component.ts @@ -10,6 +10,7 @@ import { import { ApiService } from 'jslib-common/abstractions/api.service'; import { KeyConnectorService } from 'jslib-common/abstractions/keyConnector.service'; +import { UserVerificationService } from 'jslib-common/abstractions/userVerification.service'; import { VerificationType } from 'jslib-common/enums/verificationType'; @@ -34,7 +35,8 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn private onChange: (value: Verification) => void; - constructor(private keyConnectorService: KeyConnectorService, private apiService: ApiService) { } + constructor(private keyConnectorService: KeyConnectorService, + private userVerificationService: UserVerificationService) { } async ngOnInit() { this.usesKeyConnector = await this.keyConnectorService.getUsesKeyConnector(); @@ -54,7 +56,7 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn async requestOTP() { if (this.usesKeyConnector) { this.disableRequestOTP = true; - await this.apiService.postAccountRequestOTP(); + await this.userVerificationService.requestOTP(); } } diff --git a/common/src/abstractions/userVerification.service.ts b/common/src/abstractions/userVerification.service.ts index 05c7a090..7dcf1d10 100644 --- a/common/src/abstractions/userVerification.service.ts +++ b/common/src/abstractions/userVerification.service.ts @@ -6,4 +6,5 @@ export abstract class UserVerificationService { buildRequest: (verification: Verification, requestClass?: new () => T, alreadyHashed?: boolean) => Promise; verifyUser: (verification: Verification) => Promise; + requestOTP: () => Promise; } diff --git a/common/src/models/response/identityTokenResponse.ts b/common/src/models/response/identityTokenResponse.ts index 7b128f44..c2283956 100644 --- a/common/src/models/response/identityTokenResponse.ts +++ b/common/src/models/response/identityTokenResponse.ts @@ -15,6 +15,7 @@ export class IdentityTokenResponse extends BaseResponse { kdf: KdfType; kdfIterations: number; forcePasswordReset: boolean; + apiUseKeyConnector: boolean; keyConnectorUrl: string; constructor(response: any) { @@ -31,6 +32,7 @@ export class IdentityTokenResponse extends BaseResponse { this.kdf = this.getResponseProperty('Kdf'); this.kdfIterations = this.getResponseProperty('KdfIterations'); this.forcePasswordReset = this.getResponseProperty('ForcePasswordReset'); + this.apiUseKeyConnector = this.getResponseProperty('ApiUseKeyConnector'); this.keyConnectorUrl = this.getResponseProperty('KeyConnectorUrl'); } } diff --git a/common/src/services/auth.service.ts b/common/src/services/auth.service.ts index 5c5ca225..e4f670d7 100644 --- a/common/src/services/auth.service.ts +++ b/common/src/services/auth.service.ts @@ -370,8 +370,9 @@ export class AuthService implements AuthServiceAbstraction { if (tokenResponse.keyConnectorUrl != null) { await this.keyConnectorService.getAndSetKey(tokenResponse.keyConnectorUrl); - } else if (this.environmentService.getKeyConnectorUrl() != null) { - await this.keyConnectorService.getAndSetKey(); + } else if (tokenResponse.apiUseKeyConnector) { + const keyConnectorUrl = this.environmentService.getKeyConnectorUrl(); + await this.keyConnectorService.getAndSetKey(keyConnectorUrl); } await this.cryptoService.setEncKey(tokenResponse.key); diff --git a/common/src/services/keyConnector.service.ts b/common/src/services/keyConnector.service.ts index 68aac9a2..d0b8e040 100644 --- a/common/src/services/keyConnector.service.ts +++ b/common/src/services/keyConnector.service.ts @@ -1,6 +1,5 @@ import { ApiService } from '../abstractions/api.service'; import { CryptoService } from '../abstractions/crypto.service'; -import { EnvironmentService } from '../abstractions/environment.service'; import { KeyConnectorService as KeyConnectorServiceAbstraction } from '../abstractions/keyConnector.service'; import { LogService } from '../abstractions/log.service'; import { StorageService } from '../abstractions/storage.service'; @@ -25,8 +24,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { constructor(private storageService: StorageService, private userService: UserService, private cryptoService: CryptoService, private apiService: ApiService, - private environmentService: EnvironmentService, private tokenService: TokenService, - private logService: LogService) { } + private tokenService: TokenService, private logService: LogService) { } setUsesKeyConnector(usesKeyConnector: boolean) { this.usesKeyConnector = usesKeyConnector; @@ -59,15 +57,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postConvertToKeyConnector(); } - async getAndSetKey(url?: string) { - if (url == null) { - url = this.environmentService.getKeyConnectorUrl(); - } - - if (url == null) { - throw new Error('No Key Connector URL found.'); - } - + async getAndSetKey(url: string) { try { const userKeyResponse = await this.apiService.getUserKeyFromKeyConnector(url); const keyArr = Utils.fromB64ToArray(userKeyResponse.key); diff --git a/common/src/services/userVerification.service.ts b/common/src/services/userVerification.service.ts index 7910f44a..2b94ffae 100644 --- a/common/src/services/userVerification.service.ts +++ b/common/src/services/userVerification.service.ts @@ -1,12 +1,8 @@ -import { Injectable } from '@angular/core'; - import { UserVerificationService as UserVerificationServiceAbstraction } from '../abstractions/userVerification.service'; import { ApiService } from '../abstractions/api.service'; import { CryptoService } from '../abstractions/crypto.service'; import { I18nService } from '../abstractions/i18n.service'; -import { LogService } from '../abstractions/log.service'; -import { PlatformUtilsService } from '../abstractions/platformUtils.service'; import { VerificationType } from '../enums/verificationType'; @@ -15,17 +11,13 @@ import { SecretVerificationRequest } from '../models/request/secretVerificationR import { Verification } from '../types/verification'; -@Injectable() export class UserVerificationService implements UserVerificationServiceAbstraction { constructor(private cryptoService: CryptoService, private i18nService: I18nService, - private platformUtilsService: PlatformUtilsService, private apiService: ApiService, - private logService: LogService) { } + private apiService: ApiService) { } async buildRequest(verification: Verification, requestClass?: new () => T, alreadyHashed?: boolean) { - if (verification?.secret == null || verification.secret === '') { - throw new Error('No secret provided for verification.'); - } + this.validateInput(verification); const request = requestClass != null ? new requestClass() @@ -43,28 +35,35 @@ export class UserVerificationService implements UserVerificationServiceAbstracti } async verifyUser(verification: Verification): Promise { - if (verification?.secret == null || verification.secret === '') { - throw new Error('No secret provided for verification.'); - } + this.validateInput(verification); if (verification.type === VerificationType.OTP) { const request = new VerifyOTPRequest(verification.secret); try { await this.apiService.postAccountVerifyOTP(request); } catch (e) { - this.logService.error(e); - this.platformUtilsService.showToast('error', this.i18nService.t('errorOccurred'), - this.i18nService.t('invalidVerificationCode')); - return false; + throw new Error(this.i18nService.t('invalidVerificationCode')); } } else { const passwordValid = await this.cryptoService.compareAndUpdateKeyHash(verification.secret, null); if (!passwordValid) { - this.platformUtilsService.showToast('error', this.i18nService.t('errorOccurred'), - this.i18nService.t('invalidMasterPassword')); - return false; + throw new Error(this.i18nService.t('invalidMasterPassword')); } } return true; } + + async requestOTP() { + await this.apiService.postAccountRequestOTP(); + } + + private validateInput(verification: Verification) { + if (verification?.secret == null || verification.secret === '') { + if (verification.type === VerificationType.OTP) { + throw new Error(this.i18nService.t('verificationCodeRequired')); + } else { + throw new Error(this.i18nService.t('masterPassRequired')); + } + } + } } From 720967475b37d635c18a1eb74bb3702445647b4d Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Tue, 16 Nov 2021 19:43:37 +1000 Subject: [PATCH 7/7] Update base export component for userVerificationService changes (#552) * Use new try/catch pattern in export.component * Set initial value in VerifyMasterPass component --- angular/src/components/export.component.ts | 5 +++- .../verify-master-password.component.ts | 24 ++++++++++--------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/angular/src/components/export.component.ts b/angular/src/components/export.component.ts index 8b9372c3..716f9ef0 100644 --- a/angular/src/components/export.component.ts +++ b/angular/src/components/export.component.ts @@ -69,7 +69,10 @@ export class ExportComponent implements OnInit { } const secret = this.exportForm.get('secret').value; - if (!await this.userVerificationService.verifyUser(secret)) { + try { + await this.userVerificationService.verifyUser(secret); + } catch (e) { + this.platformUtilsService.showToast('error', this.i18nService.t('errorOccurred'), e.message); return; } diff --git a/angular/src/components/verify-master-password.component.ts b/angular/src/components/verify-master-password.component.ts index 658f7f59..9d8bca63 100644 --- a/angular/src/components/verify-master-password.component.ts +++ b/angular/src/components/verify-master-password.component.ts @@ -8,7 +8,6 @@ import { NG_VALUE_ACCESSOR, } from '@angular/forms'; -import { ApiService } from 'jslib-common/abstractions/api.service'; import { KeyConnectorService } from 'jslib-common/abstractions/keyConnector.service'; import { UserVerificationService } from 'jslib-common/abstractions/userVerification.service'; @@ -40,17 +39,9 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn async ngOnInit() { this.usesKeyConnector = await this.keyConnectorService.getUsesKeyConnector(); + this.processChanges(this.secret.value); - this.secret.valueChanges.subscribe(secret => { - if (this.onChange == null) { - return; - } - - this.onChange({ - type: this.usesKeyConnector ? VerificationType.OTP : VerificationType.MasterPassword, - secret: secret, - }); - }); + this.secret.valueChanges.subscribe(secret => this.processChanges(secret)); } async requestOTP() { @@ -80,4 +71,15 @@ export class VerifyMasterPasswordComponent implements ControlValueAccessor, OnIn this.secret.enable(); } } + + private processChanges(secret: string) { + if (this.onChange == null) { + return; + } + + this.onChange({ + type: this.usesKeyConnector ? VerificationType.OTP : VerificationType.MasterPassword, + secret: secret, + }); + } }