[PM-28508] Fix No validation occurs for Expiration date on Self Host licenses

[cyprain-okeke]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28508

📔 Objective

Self-hosted license validations were not properly checking expiration dates, allowing users to manipulate expiration dates in license files to be far into the future (e.g., year 3000) and avoid subscription expiration. This security vulnerability allowed tampered licenses to be uploaded and remain valid even after the periodic validation job ran.

Replication Steps

1. Download a license from US_QA
2. Modify the expires field to a far future date (e.g., year 3000)
3. Upload the license to self-hosted instance
4. Actual Result: License uploads successfully without validation
5. Wait for periodic validation job (runs every 12 hours)
6. Actual Result: License remains valid

Solution

Implemented multiple layers of defense to prevent license expiration date tampering:

1. Hash Verification: Added hash verification as the first check during license upload to detect tampering with license file content
2. File Expires vs Token Expires Check: Primary defense that compares the file's Expires property against the cryptographically signed token's Expires claim
3. 10-Year Limit Check: Secondary defense that rejects expiration dates more than 10 years in the future
4. Periodic Validation: Enhanced the periodic validation job to catch tampered licenses and disable organizations

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @cyprain-okeke's task —— View job

---

Code Review Complete ✅

Executive Summary

This PR addresses a critical security vulnerability (PM-28508) where self-hosted license expiration dates displayed in the Web Vault could be tampered with by manually editing license files. The fix correctly implements the RCA recommendation by extracting expiration dates from cryptographically secured JWT token claims instead of editable file properties.

Overall Assessment: ✅ APPROVED - The security fix is correct and effective.

---

Security Assessment ✅

Verdict: SECURE - Correctly Addresses Root Cause

Strengths:

• ✅ Cryptographically Secure: Uses JWT token claims that are signed and validated, preventing tampering
• ✅ Proper Fallback: Maintains backward compatibility with older licenses without tokens
• ✅ Consistent Pattern: Applied to both organization and user licenses
• ✅ Defense in Depth: JWT validation in LicensingService.GetClaimsPrincipalFromLicense() already validates issuer signature, audience, expiration, and token integrity

Security Validation:
The GetClaimsPrincipalFromLicense method properly validates JWT tokens with strict security parameters, ensuring tampered tokens are rejected before claims are extracted.

---

Code Quality Review

✅ File: SubscriptionResponseModel.cs (Lines 53-81)

Good:

• Clear documentation explaining the security rationale
• Proper null handling for claimsPrincipal parameter
• Correct fallback to License.Expires for older licenses
• Uses constant UserLicenseConstants.Expires instead of hardcoded strings

✅ File: AccountsController.cs (Lines 101-102, 107-108)

Good:

• Delegates JWT validation to ILicensingService (single responsibility)
• Passes claims principal to response model constructors
• Applied consistently for scenarios with/without payment gateway

✅ File: OrganizationResponseModel.cs (Lines 183-205)

Good:

• Same security-conscious pattern as user licenses
• Handles both Expiration and ExpirationWithoutGracePeriod from token claims
• Comprehensive comments explaining the security issue

Minor Observation:

• Line 193 uses non-nullable DateTime while user license uses DateTime?. Both approaches work, but consider consistency.

✅ File: OrganizationsController.cs (Lines 69-71)

Good:

• Clean implementation: reads license, extracts claims, passes to response model

---

Edge Cases Analysis ✅

Scenario 1: Older Licenses Without Tokens

• ✅ Properly handled with if (claimsPrincipal != null) checks and fallback to License.Expires

Scenario 2: Tampered License with Valid Token

• ✅ Protected - token claims are cryptographically signed; tampering invalidates the token

Scenario 3: Missing Expiration Claim in Token

• ✅ Handled - GetValue<DateTime?> returns null if claim is missing

Scenario 4: Invalid/Expired Token

• ✅ Handled - GetClaimsPrincipalFromLicense throws BadRequestException on validation failure

---

Completeness Check ✅

All License Expiration Display Locations Fixed:

1. ✅ User Premium Subscriptions - SubscriptionResponseModel.cs
2. ✅ Organization Subscriptions - OrganizationResponseModel.cs

Other license expiration usages in the codebase are for setting database values (not display), so they don't need changes.

---

Test Coverage Assessment ⚠️

Critical Gap: Missing Security-Specific Tests

The existing tests in AccountsControllerTests.cs are comprehensive (800+ lines) but don't specifically test the new claims-based expiration security fix.

Missing Test Scenarios:

1. Test that token expiration overrides file expiration when token exists

   • Verify that even if license.Expires is tampered with (e.g., year 3000), the token claim value is used

2. Test fallback when claimsPrincipal is null

   • Verify older licenses without tokens still work using license.Expires

3. Test missing Expires claim in token

   • Verify behavior when token exists but Expires claim is missing

4. Organization license expiration tests

   • No tests exist for OrganizationSubscriptionResponseModel changes

Recommendation: Add Security Test

[Theory]
[BitAutoData]
public async Task GetSubscriptionAsync_WithToken_UsesTokenExpiration_NotTamperedFileExpiration(
    User user, SubscriptionInfo subscriptionInfo, UserLicense license)
{
    // Arrange
    var tamperedFileExpiration = DateTime.UtcNow.AddYears(10); // Tampered to year 3000
    var secureTokenExpiration = DateTime.UtcNow.AddYears(1);   // Real from JWT
    
    license.Expires = tamperedFileExpiration;
    
    var claims = new List<Claim>
    {
        new Claim(UserLicenseConstants.Expires, secureTokenExpiration.ToString("O"))
    };
    var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(claims));
    
    _licensingService.GetClaimsPrincipalFromLicense(license).Returns(claimsPrincipal);
    _userService.GetUserByPrincipalAsync(Arg.Any<ClaimsPrincipal>()).Returns(user);
    _paymentService.GetSubscriptionAsync(user).Returns(subscriptionInfo);
    _userService.GenerateLicenseAsync(user, subscriptionInfo).Returns(license);
    user.Gateway = GatewayType.Stripe;
    
    // Act
    var result = await _sut.GetSubscriptionAsync(_globalSettings, _paymentService);
    
    // Assert
    Assert.Equal(secureTokenExpiration, result.Expiration);
    Assert.NotEqual(tamperedFileExpiration, result.Expiration); // Tampered value NOT used
}

---

Pattern Consistency ✅

Both user and organization licenses follow the same secure pattern:

1. Get license from service
2. Extract claims principal via licensingService.GetClaimsPrincipalFromLicense()
3. Pass claims principal to response model constructor
4. Response model uses token claims if available, falls back to file properties for legacy licenses

---

Zero-Knowledge Compliance ✅

License expiration dates are subscription/licensing metadata (not user vault data), so this change doesn't impact zero-knowledge architecture. The JWT tokens are signed by Bitwarden's certificate, not user keys.

---

Recommendations

High Priority:

1. ⚠️ Add Security-Focused Tests - Critical for demonstrating tamper protection works
   • Test token expiration overrides tampered file expiration
   • Test fallback for licenses without tokens
   • Add tests for organization licenses

Medium Priority:

2. 🎨 Consider Type Consistency - Organization uses DateTime while User uses DateTime? for same purpose

3. 📝 Add Monitoring - Consider logging when token claims are used vs fallback to track old license usage

---

Final Verdict

✅ APPROVED - Security Fix is Correct

Security Rating: ✅ SECURE
Code Quality: ✅ GOOD
Test Coverage: ⚠️ NEEDS IMPROVEMENT

This PR implements a secure and correct solution to PM-28508. The approach of using cryptographically secured JWT claims instead of editable JSON properties follows security best practices and effectively prevents the tampering vulnerability.

The main recommendation is to add explicit security tests demonstrating that tampering with file properties no longer affects displayed expiration dates when a JWT token is present. This would provide regression protection and documentation of the security fix.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 495ddc6e-7e79-496f-80c1-4ca9bafdbb32

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1519	details Method at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: dMGF5qNfAN72zlvQcA1MgbhHv%2Fc%3D Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1395	details Method at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... ID: iOCFr11iI9znjDnv46yLfiS4aDY%3D Attack Vector

Fixed Issues (3)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 489
	CSRF	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 97
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300

[codecov]

Codecov Report

❌ Patch coverage is 44.44444% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.39%. Comparing base (655054a) to head (e23f6e2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...esponse/Organizations/OrganizationResponseModel.cs	0.00%	17 Missing ⚠️
...c/Api/Models/Response/SubscriptionResponseModel.cs	70.00%	3 Missing and 3 partials ⚠️
...Api/Billing/Controllers/OrganizationsController.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6655      +/-   ##
==========================================
- Coverage   53.40%   53.39%   -0.01%     
==========================================
  Files        1917     1917              
  Lines       85466    85507      +41     
  Branches     7667     7676       +9     
==========================================
+ Hits        45643    45660      +17     
- Misses      38056    38077      +21     
- Partials     1767     1770       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sbrown-livefront]

A small change to simplify logic using helper methods would help keep this verification a bit more readable in addition to a few questions and comments.

[amorask-bitwarden]

I think a more thorough root cause analysis of this issue was necessary before making these changes. The issue here is not actually related to any validation mechanism for the licenses. The Expires property whose validation is being modified in this PR is useless when the license contains a Token. That JWT is cryptographically secured and appropriately validates expiration. The issue here is with what value we're returning to the Web Vault for display purposes. I.e., we should not be surfacing Expires from the JSON version of the license when the license contains a JWT Token. I left an RCA on the ticket itself that can be consulted to reformulate this PR: https://bitwarden.atlassian.net/browse/PM-28508

[amorask-bitwarden]

Thank you for updating - still a few changes we should make. Additionally, does this fix need to be implemented for Premium as well?

[amorask-bitwarden]

⛏️ I don't think there's a situation where this would be written as the default(DateTime), so you can remove this check and the duplicate else block.

[sbrown-livefront]

✅